| Message ID | 1679911940-4727-4-git-send-email-quic_linyyuan@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/6] usb: gadget: ffs: remove ENTER() macro | expand |
On Mon, Mar 27, 2023 at 06:12:18PM +0800, Linyu Yuan wrote: > It is known that dev_vdbg() macro can accept NULL or non-NULL dev pointer. > > Add a struct device *dev member in struct ffs_data, set it to NULL before > binding or after unbinding to a usb_gadget, set it reference of usb_gadget > ->dev when bind success. > > Then it can help replace private pr_vdebug() to dev_vdbg() consistently. > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > --- > v3: new patch in this version > > drivers/usb/gadget/function/f_fs.c | 3 +++ > drivers/usb/gadget/function/u_fs.h | 1 + > 2 files changed, 4 insertions(+) > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c > index a4051c8..25461f1 100644 > --- a/drivers/usb/gadget/function/f_fs.c > +++ b/drivers/usb/gadget/function/f_fs.c > @@ -1722,6 +1722,7 @@ static struct ffs_data *ffs_data_new(const char *dev_name) > return NULL; > } > > + ffs->dev = NULL; > refcount_set(&ffs->ref, 1); > atomic_set(&ffs->opened, 0); > ffs->state = FFS_READ_DESCRIPTORS; > @@ -1831,6 +1832,7 @@ static int functionfs_bind(struct ffs_data *ffs, struct usb_composite_dev *cdev) > } > > ffs->gadget = cdev->gadget; > + ffs->dev = &cdev->gadget->dev; > ffs_data_get(ffs); > return 0; > } > @@ -1843,6 +1845,7 @@ static void functionfs_unbind(struct ffs_data *ffs) > mutex_lock(&ffs->mutex); > usb_ep_free_request(ffs->gadget->ep0, ffs->ep0req); > ffs->ep0req = NULL; > + ffs->dev = NULL; > ffs->gadget = NULL; > clear_bit(FFS_FL_BOUND, &ffs->flags); > mutex_unlock(&ffs->mutex); > diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h > index 4b3365f..c5f6167 100644 > --- a/drivers/usb/gadget/function/u_fs.h > +++ b/drivers/usb/gadget/function/u_fs.h > @@ -146,6 +146,7 @@ enum ffs_setup_state { > > struct ffs_data { > struct usb_gadget *gadget; > + struct device *dev; No, sorry, this is not correct. You already have a struct device right there in the struct usb_gadget. Use that one instead, as you are just setting this pointer to the same value (see above where you set it.) thanks, greg k-h
On 3/29/2023 2:53 PM, Greg Kroah-Hartman wrote: > On Mon, Mar 27, 2023 at 06:12:18PM +0800, Linyu Yuan wrote: >> It is known that dev_vdbg() macro can accept NULL or non-NULL dev pointer. >> >> Add a struct device *dev member in struct ffs_data, set it to NULL before >> binding or after unbinding to a usb_gadget, set it reference of usb_gadget >> ->dev when bind success. >> >> Then it can help replace private pr_vdebug() to dev_vdbg() consistently. >> >> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> >> --- >> v3: new patch in this version >> >> drivers/usb/gadget/function/f_fs.c | 3 +++ >> drivers/usb/gadget/function/u_fs.h | 1 + >> 2 files changed, 4 insertions(+) >> >> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c >> index a4051c8..25461f1 100644 >> --- a/drivers/usb/gadget/function/f_fs.c >> +++ b/drivers/usb/gadget/function/f_fs.c >> @@ -1722,6 +1722,7 @@ static struct ffs_data *ffs_data_new(const char *dev_name) >> return NULL; >> } >> >> + ffs->dev = NULL; >> refcount_set(&ffs->ref, 1); >> atomic_set(&ffs->opened, 0); >> ffs->state = FFS_READ_DESCRIPTORS; >> @@ -1831,6 +1832,7 @@ static int functionfs_bind(struct ffs_data *ffs, struct usb_composite_dev *cdev) >> } >> >> ffs->gadget = cdev->gadget; >> + ffs->dev = &cdev->gadget->dev; >> ffs_data_get(ffs); >> return 0; >> } >> @@ -1843,6 +1845,7 @@ static void functionfs_unbind(struct ffs_data *ffs) >> mutex_lock(&ffs->mutex); >> usb_ep_free_request(ffs->gadget->ep0, ffs->ep0req); >> ffs->ep0req = NULL; >> + ffs->dev = NULL; >> ffs->gadget = NULL; >> clear_bit(FFS_FL_BOUND, &ffs->flags); >> mutex_unlock(&ffs->mutex); >> diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h >> index 4b3365f..c5f6167 100644 >> --- a/drivers/usb/gadget/function/u_fs.h >> +++ b/drivers/usb/gadget/function/u_fs.h >> @@ -146,6 +146,7 @@ enum ffs_setup_state { >> >> struct ffs_data { >> struct usb_gadget *gadget; >> + struct device *dev; > No, sorry, this is not correct. > > You already have a struct device right there in the struct usb_gadget. > Use that one instead, as you are just setting this pointer to the same > value (see above where you set it.) just want to use consistent dev_(v)dbg() related macro, to avoid reference usb_gadget->dev when usb_gadget is NULL. > > thanks, > > greg k-h
On Wed, Mar 29, 2023 at 03:00:54PM +0800, Linyu Yuan wrote: > > On 3/29/2023 2:53 PM, Greg Kroah-Hartman wrote: > > On Mon, Mar 27, 2023 at 06:12:18PM +0800, Linyu Yuan wrote: > > > It is known that dev_vdbg() macro can accept NULL or non-NULL dev pointer. > > > > > > Add a struct device *dev member in struct ffs_data, set it to NULL before > > > binding or after unbinding to a usb_gadget, set it reference of usb_gadget > > > ->dev when bind success. > > > > > > Then it can help replace private pr_vdebug() to dev_vdbg() consistently. > > > > > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > > > --- > > > v3: new patch in this version > > > > > > drivers/usb/gadget/function/f_fs.c | 3 +++ > > > drivers/usb/gadget/function/u_fs.h | 1 + > > > 2 files changed, 4 insertions(+) > > > > > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c > > > index a4051c8..25461f1 100644 > > > --- a/drivers/usb/gadget/function/f_fs.c > > > +++ b/drivers/usb/gadget/function/f_fs.c > > > @@ -1722,6 +1722,7 @@ static struct ffs_data *ffs_data_new(const char *dev_name) > > > return NULL; > > > } > > > + ffs->dev = NULL; > > > refcount_set(&ffs->ref, 1); > > > atomic_set(&ffs->opened, 0); > > > ffs->state = FFS_READ_DESCRIPTORS; > > > @@ -1831,6 +1832,7 @@ static int functionfs_bind(struct ffs_data *ffs, struct usb_composite_dev *cdev) > > > } > > > ffs->gadget = cdev->gadget; > > > + ffs->dev = &cdev->gadget->dev; > > > ffs_data_get(ffs); > > > return 0; > > > } > > > @@ -1843,6 +1845,7 @@ static void functionfs_unbind(struct ffs_data *ffs) > > > mutex_lock(&ffs->mutex); > > > usb_ep_free_request(ffs->gadget->ep0, ffs->ep0req); > > > ffs->ep0req = NULL; > > > + ffs->dev = NULL; > > > ffs->gadget = NULL; > > > clear_bit(FFS_FL_BOUND, &ffs->flags); > > > mutex_unlock(&ffs->mutex); > > > diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h > > > index 4b3365f..c5f6167 100644 > > > --- a/drivers/usb/gadget/function/u_fs.h > > > +++ b/drivers/usb/gadget/function/u_fs.h > > > @@ -146,6 +146,7 @@ enum ffs_setup_state { > > > struct ffs_data { > > > struct usb_gadget *gadget; > > > + struct device *dev; > > No, sorry, this is not correct. > > > > You already have a struct device right there in the struct usb_gadget. > > Use that one instead, as you are just setting this pointer to the same > > value (see above where you set it.) > > > just want to use consistent dev_(v)dbg() related macro, to avoid reference > usb_gadget->dev > > when usb_gadget is NULL. When will usb_gadget be NULL when you want to print out logging messages? You shouldn't be printing out anything during that time anyway, right? thanks, greg k-h
On 3/29/2023 3:31 PM, Greg Kroah-Hartman wrote: > On Wed, Mar 29, 2023 at 03:00:54PM +0800, Linyu Yuan wrote: >> On 3/29/2023 2:53 PM, Greg Kroah-Hartman wrote: >>> On Mon, Mar 27, 2023 at 06:12:18PM +0800, Linyu Yuan wrote: >>>> It is known that dev_vdbg() macro can accept NULL or non-NULL dev pointer. >>>> >>>> Add a struct device *dev member in struct ffs_data, set it to NULL before >>>> binding or after unbinding to a usb_gadget, set it reference of usb_gadget >>>> ->dev when bind success. >>>> >>>> Then it can help replace private pr_vdebug() to dev_vdbg() consistently. >>>> >>>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> >>>> --- >>>> v3: new patch in this version >>>> >>>> drivers/usb/gadget/function/f_fs.c | 3 +++ >>>> drivers/usb/gadget/function/u_fs.h | 1 + >>>> 2 files changed, 4 insertions(+) >>>> >>>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c >>>> index a4051c8..25461f1 100644 >>>> --- a/drivers/usb/gadget/function/f_fs.c >>>> +++ b/drivers/usb/gadget/function/f_fs.c >>>> @@ -1722,6 +1722,7 @@ static struct ffs_data *ffs_data_new(const char *dev_name) >>>> return NULL; >>>> } >>>> + ffs->dev = NULL; >>>> refcount_set(&ffs->ref, 1); >>>> atomic_set(&ffs->opened, 0); >>>> ffs->state = FFS_READ_DESCRIPTORS; >>>> @@ -1831,6 +1832,7 @@ static int functionfs_bind(struct ffs_data *ffs, struct usb_composite_dev *cdev) >>>> } >>>> ffs->gadget = cdev->gadget; >>>> + ffs->dev = &cdev->gadget->dev; >>>> ffs_data_get(ffs); >>>> return 0; >>>> } >>>> @@ -1843,6 +1845,7 @@ static void functionfs_unbind(struct ffs_data *ffs) >>>> mutex_lock(&ffs->mutex); >>>> usb_ep_free_request(ffs->gadget->ep0, ffs->ep0req); >>>> ffs->ep0req = NULL; >>>> + ffs->dev = NULL; >>>> ffs->gadget = NULL; >>>> clear_bit(FFS_FL_BOUND, &ffs->flags); >>>> mutex_unlock(&ffs->mutex); >>>> diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h >>>> index 4b3365f..c5f6167 100644 >>>> --- a/drivers/usb/gadget/function/u_fs.h >>>> +++ b/drivers/usb/gadget/function/u_fs.h >>>> @@ -146,6 +146,7 @@ enum ffs_setup_state { >>>> struct ffs_data { >>>> struct usb_gadget *gadget; >>>> + struct device *dev; >>> No, sorry, this is not correct. >>> >>> You already have a struct device right there in the struct usb_gadget. >>> Use that one instead, as you are just setting this pointer to the same >>> value (see above where you set it.) >> >> just want to use consistent dev_(v)dbg() related macro, to avoid reference >> usb_gadget->dev >> >> when usb_gadget is NULL. > When will usb_gadget be NULL when you want to print out logging > messages? You shouldn't be printing out anything during that time > anyway, right? when usb_gadget is NULL, there could be debug message because user space application can start configure the ffs instance (like adb ...) for USB interface/endpoint/string descriptor. as dev_dbg related macro is safe to accept NULL, there is no need find out when will usb_gadget is NULL and when will it a valid pointer. > > thanks, > > greg k-h
On Wed, Mar 29, 2023 at 03:46:45PM +0800, Linyu Yuan wrote: > > On 3/29/2023 3:31 PM, Greg Kroah-Hartman wrote: > > On Wed, Mar 29, 2023 at 03:00:54PM +0800, Linyu Yuan wrote: > > > On 3/29/2023 2:53 PM, Greg Kroah-Hartman wrote: > > > > On Mon, Mar 27, 2023 at 06:12:18PM +0800, Linyu Yuan wrote: > > > > > It is known that dev_vdbg() macro can accept NULL or non-NULL dev pointer. > > > > > > > > > > Add a struct device *dev member in struct ffs_data, set it to NULL before > > > > > binding or after unbinding to a usb_gadget, set it reference of usb_gadget > > > > > ->dev when bind success. > > > > > > > > > > Then it can help replace private pr_vdebug() to dev_vdbg() consistently. > > > > > > > > > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > > > > > --- > > > > > v3: new patch in this version > > > > > > > > > > drivers/usb/gadget/function/f_fs.c | 3 +++ > > > > > drivers/usb/gadget/function/u_fs.h | 1 + > > > > > 2 files changed, 4 insertions(+) > > > > > > > > > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c > > > > > index a4051c8..25461f1 100644 > > > > > --- a/drivers/usb/gadget/function/f_fs.c > > > > > +++ b/drivers/usb/gadget/function/f_fs.c > > > > > @@ -1722,6 +1722,7 @@ static struct ffs_data *ffs_data_new(const char *dev_name) > > > > > return NULL; > > > > > } > > > > > + ffs->dev = NULL; > > > > > refcount_set(&ffs->ref, 1); > > > > > atomic_set(&ffs->opened, 0); > > > > > ffs->state = FFS_READ_DESCRIPTORS; > > > > > @@ -1831,6 +1832,7 @@ static int functionfs_bind(struct ffs_data *ffs, struct usb_composite_dev *cdev) > > > > > } > > > > > ffs->gadget = cdev->gadget; > > > > > + ffs->dev = &cdev->gadget->dev; > > > > > ffs_data_get(ffs); > > > > > return 0; > > > > > } > > > > > @@ -1843,6 +1845,7 @@ static void functionfs_unbind(struct ffs_data *ffs) > > > > > mutex_lock(&ffs->mutex); > > > > > usb_ep_free_request(ffs->gadget->ep0, ffs->ep0req); > > > > > ffs->ep0req = NULL; > > > > > + ffs->dev = NULL; > > > > > ffs->gadget = NULL; > > > > > clear_bit(FFS_FL_BOUND, &ffs->flags); > > > > > mutex_unlock(&ffs->mutex); > > > > > diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h > > > > > index 4b3365f..c5f6167 100644 > > > > > --- a/drivers/usb/gadget/function/u_fs.h > > > > > +++ b/drivers/usb/gadget/function/u_fs.h > > > > > @@ -146,6 +146,7 @@ enum ffs_setup_state { > > > > > struct ffs_data { > > > > > struct usb_gadget *gadget; > > > > > + struct device *dev; > > > > No, sorry, this is not correct. > > > > > > > > You already have a struct device right there in the struct usb_gadget. > > > > Use that one instead, as you are just setting this pointer to the same > > > > value (see above where you set it.) > > > > > > just want to use consistent dev_(v)dbg() related macro, to avoid reference > > > usb_gadget->dev > > > > > > when usb_gadget is NULL. > > When will usb_gadget be NULL when you want to print out logging > > messages? You shouldn't be printing out anything during that time > > anyway, right? > > > when usb_gadget is NULL, there could be debug message because user space > application > > can start configure the ffs instance (like adb ...) for USB > interface/endpoint/string descriptor. But there is a "real" device down there somewhere as there would not be any way for the driver to be able to talk to the hardware. So please use that. > as dev_dbg related macro is safe to accept NULL, there is no need find out > when will > > usb_gadget is NULL and when will it a valid pointer. Don't abuse dev_dbg() like this, that's not going to help out much overall, and makes no sense to convert to something that is going to print out incorrect messages (again, there is a device there.) thanks, greg k-h
On 3/29/2023 4:21 PM, Greg Kroah-Hartman wrote: > On Wed, Mar 29, 2023 at 03:46:45PM +0800, Linyu Yuan wrote: >> On 3/29/2023 3:31 PM, Greg Kroah-Hartman wrote: >>> On Wed, Mar 29, 2023 at 03:00:54PM +0800, Linyu Yuan wrote: >>>> On 3/29/2023 2:53 PM, Greg Kroah-Hartman wrote: >>>>> On Mon, Mar 27, 2023 at 06:12:18PM +0800, Linyu Yuan wrote: >>>>>> It is known that dev_vdbg() macro can accept NULL or non-NULL dev pointer. >>>>>> >>>>>> Add a struct device *dev member in struct ffs_data, set it to NULL before >>>>>> binding or after unbinding to a usb_gadget, set it reference of usb_gadget >>>>>> ->dev when bind success. >>>>>> >>>>>> Then it can help replace private pr_vdebug() to dev_vdbg() consistently. >>>>>> >>>>>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> >>>>>> --- >>>>>> v3: new patch in this version >>>>>> >>>>>> drivers/usb/gadget/function/f_fs.c | 3 +++ >>>>>> drivers/usb/gadget/function/u_fs.h | 1 + >>>>>> 2 files changed, 4 insertions(+) >>>>>> >>>>>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c >>>>>> index a4051c8..25461f1 100644 >>>>>> --- a/drivers/usb/gadget/function/f_fs.c >>>>>> +++ b/drivers/usb/gadget/function/f_fs.c >>>>>> @@ -1722,6 +1722,7 @@ static struct ffs_data *ffs_data_new(const char *dev_name) >>>>>> return NULL; >>>>>> } >>>>>> + ffs->dev = NULL; >>>>>> refcount_set(&ffs->ref, 1); >>>>>> atomic_set(&ffs->opened, 0); >>>>>> ffs->state = FFS_READ_DESCRIPTORS; >>>>>> @@ -1831,6 +1832,7 @@ static int functionfs_bind(struct ffs_data *ffs, struct usb_composite_dev *cdev) >>>>>> } >>>>>> ffs->gadget = cdev->gadget; >>>>>> + ffs->dev = &cdev->gadget->dev; >>>>>> ffs_data_get(ffs); >>>>>> return 0; >>>>>> } >>>>>> @@ -1843,6 +1845,7 @@ static void functionfs_unbind(struct ffs_data *ffs) >>>>>> mutex_lock(&ffs->mutex); >>>>>> usb_ep_free_request(ffs->gadget->ep0, ffs->ep0req); >>>>>> ffs->ep0req = NULL; >>>>>> + ffs->dev = NULL; >>>>>> ffs->gadget = NULL; >>>>>> clear_bit(FFS_FL_BOUND, &ffs->flags); >>>>>> mutex_unlock(&ffs->mutex); >>>>>> diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h >>>>>> index 4b3365f..c5f6167 100644 >>>>>> --- a/drivers/usb/gadget/function/u_fs.h >>>>>> +++ b/drivers/usb/gadget/function/u_fs.h >>>>>> @@ -146,6 +146,7 @@ enum ffs_setup_state { >>>>>> struct ffs_data { >>>>>> struct usb_gadget *gadget; >>>>>> + struct device *dev; >>>>> No, sorry, this is not correct. >>>>> >>>>> You already have a struct device right there in the struct usb_gadget. >>>>> Use that one instead, as you are just setting this pointer to the same >>>>> value (see above where you set it.) >>>> just want to use consistent dev_(v)dbg() related macro, to avoid reference >>>> usb_gadget->dev >>>> >>>> when usb_gadget is NULL. >>> When will usb_gadget be NULL when you want to print out logging >>> messages? You shouldn't be printing out anything during that time >>> anyway, right? >> >> when usb_gadget is NULL, there could be debug message because user space >> application >> >> can start configure the ffs instance (like adb ...) for USB >> interface/endpoint/string descriptor. > But there is a "real" device down there somewhere as there would not be > any way for the driver to be able to talk to the hardware. So please > use that. no real device before bind, it only has a file. > >> as dev_dbg related macro is safe to accept NULL, there is no need find out >> when will >> >> usb_gadget is NULL and when will it a valid pointer. > Don't abuse dev_dbg() like this, that's not going to help out much > overall, and makes no sense to convert to something that is going to > print out incorrect messages (again, there is a device there.) > > thanks, > > greg k-h
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c index a4051c8..25461f1 100644 --- a/drivers/usb/gadget/function/f_fs.c +++ b/drivers/usb/gadget/function/f_fs.c @@ -1722,6 +1722,7 @@ static struct ffs_data *ffs_data_new(const char *dev_name) return NULL; } + ffs->dev = NULL; refcount_set(&ffs->ref, 1); atomic_set(&ffs->opened, 0); ffs->state = FFS_READ_DESCRIPTORS; @@ -1831,6 +1832,7 @@ static int functionfs_bind(struct ffs_data *ffs, struct usb_composite_dev *cdev) } ffs->gadget = cdev->gadget; + ffs->dev = &cdev->gadget->dev; ffs_data_get(ffs); return 0; } @@ -1843,6 +1845,7 @@ static void functionfs_unbind(struct ffs_data *ffs) mutex_lock(&ffs->mutex); usb_ep_free_request(ffs->gadget->ep0, ffs->ep0req); ffs->ep0req = NULL; + ffs->dev = NULL; ffs->gadget = NULL; clear_bit(FFS_FL_BOUND, &ffs->flags); mutex_unlock(&ffs->mutex); diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h index 4b3365f..c5f6167 100644 --- a/drivers/usb/gadget/function/u_fs.h +++ b/drivers/usb/gadget/function/u_fs.h @@ -146,6 +146,7 @@ enum ffs_setup_state { struct ffs_data { struct usb_gadget *gadget; + struct device *dev; /* * Protect access read/write operations, only one read/write
It is known that dev_vdbg() macro can accept NULL or non-NULL dev pointer. Add a struct device *dev member in struct ffs_data, set it to NULL before binding or after unbinding to a usb_gadget, set it reference of usb_gadget ->dev when bind success. Then it can help replace private pr_vdebug() to dev_vdbg() consistently. Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> --- v3: new patch in this version drivers/usb/gadget/function/f_fs.c | 3 +++ drivers/usb/gadget/function/u_fs.h | 1 + 2 files changed, 4 insertions(+)