Re: [PATCH v3 4/6] usb: gadget: f_fs: add a device reference of usb_gadget->dev

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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