Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1620017 > unrolled thread

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same  gadget device Alan Stern <stern@rowland.harvard.edu> - 2017-04-10 17:20 +0200
    Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device Felipe Balbi <balbi@kernel.org> - 2017-04-11 09:40 +0200
      Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same  gadget device Alan Stern <stern@rowland.harvard.edu> - 2017-04-11 16:20 +0200
        Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same  gadget device Greg KH <greg@kroah.com> - 2017-04-11 16:20 +0200
          Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device Felipe Balbi <balbi@kernel.org> - 2017-04-12 08:10 +0200
            Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same  gadget device Greg KH <greg@kroah.com> - 2017-04-12 08:50 +0200
              Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device Felipe Balbi <balbi@kernel.org> - 2017-04-12 09:40 +0200
            Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same  gadget device Alan Stern <stern@rowland.harvard.edu> - 2017-04-12 16:40 +0200

#1620017 — Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

On Mon, 10 Apr 2017, Felipe Balbi wrote:

> Hi,
> 
> Alan Stern <stern@rowland.harvard.edu> writes:
> > On Wed, 5 Apr 2017, Felipe Balbi wrote:
> >
> >> >> >> --- a/drivers/usb/gadget/udc/core.c
> >> >> >> +++ b/drivers/usb/gadget/udc/core.c
> >> >> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
> >> >> >>  	flush_work(&gadget->work);
> >> >> >>  	device_unregister(&udc->dev);
> >> >> >>  	device_unregister(&gadget->dev);
> >> >> >> +	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
> >> >> >>  }
> >> >> >>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
> >> >> >
> >> >> > Isn't this dangerous?  It's quite possible that the device_unregister() 
> >> >> 
> >> >> not on the gadget API, no.
> >> >> 
> >> >> > call on the previous line invokes the gadget->dev.release callback, 
> >> >> > which might deallocate gadget.  If that happens, your new memset will 
> >> >> > oops.
> >> >> 
> >> >> that won't happen. struct usb_gadget is a member of the UDC's private
> >> >> structure, like this:
> >> >> 
> >> >> struct dwc3 {
> >> >> 	[...]
> >> >> 	struct usb_gadget	gadget;
> >> >> 	struct usb_gadget_driver *gadget_driver;
> >> >> 	[...]
> >> >> };
> >> >
> >> > Yes.  So what?  Can't the UDC driver use the refcount inside struct 
> >> > usb_gadget to control the lifetime of its private structure?
> >> 
> >> nope, not being used. At least not yet.
> >
> > I'm not convinced (yet)...
> >
> >> > (By the way, can you tell what's going on in net2280.c?  I must be
> >> > missing something; it looks like gadget_release() would quickly run
> >> > into problems because it calls dev_get_drvdata() for &gadget->dev, but
> >> > net2280_probe() never calls dev_set_drvdata() for that device.  
> >> > Furthermore, net2280_remove() continues to reference the net2280 struct
> >> > after calling usb_del_gadget_udc(), and it never does seem to do a
> >> > final put.)
> >> 
> >> static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >> {
> >> 	struct net2280		*dev;
> >> 	unsigned long		resource, len;
> >> 	void			__iomem *base = NULL;
> >> 	int			retval, i;
> >> 
> >> 	/* alloc, and start init */
> >> 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> >> 	if (dev == NULL) {
> >> 		retval = -ENOMEM;
> >> 		goto done;
> >> 	}
> >> 
> >> 	pci_set_drvdata(pdev, dev);
> >> 	^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > That sets the driver data in the struct pci_dev, not in
> > dev->gadget.dev.  As far as I can see, _nothing_ in the driver sets the 
> > driver data in dev->gadget.dev.
> 
> hmmm, indeed. The same is happening with other callers of
> usb_add_gadget_udc_release().
> 
> I guess this should be enough?
> 
> @@ -3557,7 +3557,7 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
>  
>  static void gadget_release(struct device *_dev)
>  {
> -	struct net2280	*dev = dev_get_drvdata(_dev);
> +	struct net2280	*dev = dev_get_drvdata(_dev->parent);
>  
>  	kfree(dev);
>  }

Oddly enough, yes.  But it doesn't explain why this code doesn't blow 
up every time it gets called, in its current form.

And it doesn't help with the fact that net2280_remove() continues to 
access the private data structure after calling usb_del_gadget_udc().  
Strictly speaking, that routine should do

	get_device(&dev->gadget.dev);

at the start, with a corresponding put_device() at the end.

There's another problem.  Suppose a call to 
usb_add_gadget_udc_release() fails.  At the end of that routine, the 
error pathway does put_device(&gadget->dev).  This will invoke the 
release callback, deallocating the private data structure without 
giving the caller (i.e., the UDC driver) a chance to clean up.

Alan Stern

[toc] | [next] | [standalone]

#1620985 — Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Multipart message — attachments visible in raw view] — view raw

Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
>> >> >> >> --- a/drivers/usb/gadget/udc/core.c
>> >> >> >> +++ b/drivers/usb/gadget/udc/core.c
>> >> >> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
>> >> >> >>  	flush_work(&gadget->work);
>> >> >> >>  	device_unregister(&udc->dev);
>> >> >> >>  	device_unregister(&gadget->dev);
>> >> >> >> +	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
>> >> >> >>  }
>> >> >> >>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>> >> >> >
>> >> >> > Isn't this dangerous?  It's quite possible that the device_unregister() 
>> >> >> 
>> >> >> not on the gadget API, no.
>> >> >> 
>> >> >> > call on the previous line invokes the gadget->dev.release callback, 
>> >> >> > which might deallocate gadget.  If that happens, your new memset will 
>> >> >> > oops.
>> >> >> 
>> >> >> that won't happen. struct usb_gadget is a member of the UDC's private
>> >> >> structure, like this:
>> >> >> 
>> >> >> struct dwc3 {
>> >> >> 	[...]
>> >> >> 	struct usb_gadget	gadget;
>> >> >> 	struct usb_gadget_driver *gadget_driver;
>> >> >> 	[...]
>> >> >> };
>> >> >
>> >> > Yes.  So what?  Can't the UDC driver use the refcount inside struct 
>> >> > usb_gadget to control the lifetime of its private structure?
>> >> 
>> >> nope, not being used. At least not yet.
>> >
>> > I'm not convinced (yet)...
>> >
>> >> > (By the way, can you tell what's going on in net2280.c?  I must be
>> >> > missing something; it looks like gadget_release() would quickly run
>> >> > into problems because it calls dev_get_drvdata() for &gadget->dev, but
>> >> > net2280_probe() never calls dev_set_drvdata() for that device.  
>> >> > Furthermore, net2280_remove() continues to reference the net2280 struct
>> >> > after calling usb_del_gadget_udc(), and it never does seem to do a
>> >> > final put.)
>> >> 
>> >> static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> >> {
>> >> 	struct net2280		*dev;
>> >> 	unsigned long		resource, len;
>> >> 	void			__iomem *base = NULL;
>> >> 	int			retval, i;
>> >> 
>> >> 	/* alloc, and start init */
>> >> 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>> >> 	if (dev == NULL) {
>> >> 		retval = -ENOMEM;
>> >> 		goto done;
>> >> 	}
>> >> 
>> >> 	pci_set_drvdata(pdev, dev);
>> >> 	^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> >
>> > That sets the driver data in the struct pci_dev, not in
>> > dev->gadget.dev.  As far as I can see, _nothing_ in the driver sets the 
>> > driver data in dev->gadget.dev.
>> 
>> hmmm, indeed. The same is happening with other callers of
>> usb_add_gadget_udc_release().
>> 
>> I guess this should be enough?
>> 
>> @@ -3557,7 +3557,7 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
>>  
>>  static void gadget_release(struct device *_dev)
>>  {
>> -	struct net2280	*dev = dev_get_drvdata(_dev);
>> +	struct net2280	*dev = dev_get_drvdata(_dev->parent);
>>  
>>  	kfree(dev);
>>  }
>
> Oddly enough, yes.  But it doesn't explain why this code doesn't blow 
> up every time it gets called, in its current form.

Well, it does :-)

dev_get_drvdata(_dev) -> NULL -> kfree(NULL)

We're just leaking memory. I guess a patch like below would be best:

diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c
index 3828c2ec8623..4dc04253da61 100644
--- a/drivers/usb/gadget/udc/net2280.c
+++ b/drivers/usb/gadget/udc/net2280.c
@@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
 
 /*-------------------------------------------------------------------------*/
 
-static void gadget_release(struct device *_dev)
-{
-	struct net2280	*dev = dev_get_drvdata(_dev);
-
-	kfree(dev);
-}
-
 /* tear down the binding between this driver and the pci device */
 
 static void net2280_remove(struct pci_dev *pdev)
@@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev)
 	device_remove_file(&pdev->dev, &dev_attr_registers);
 
 	ep_info(dev, "unbind\n");
+
+	kfree(dev);
 }
 
 /* wrap this driver around the specified device, but
@@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (retval)
 		goto done;
 
-	retval = usb_add_gadget_udc_release(&pdev->dev, &dev->gadget,
-			gadget_release);
+	retval = usb_add_gadget_udc(&pdev->dev, &dev->gadget);
 	if (retval)
 		goto done;
 	return 0;

> And it doesn't help with the fact that net2280_remove() continues to 
> access the private data structure after calling usb_del_gadget_udc().  
> Strictly speaking, that routine should do
>
> 	get_device(&dev->gadget.dev);
>
> at the start, with a corresponding put_device() at the end.
>
> There's another problem.  Suppose a call to 
> usb_add_gadget_udc_release() fails.  At the end of that routine, the 
> error pathway does put_device(&gadget->dev).  This will invoke the 
> release callback, deallocating the private data structure without 
> giving the caller (i.e., the UDC driver) a chance to clean up.

it won't deallocate anything :-) dev_set_drvdata() was never called,
we will endup with kfree(NULL) which is safe and just silently returns.

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1621335

On Tue, 11 Apr 2017, Felipe Balbi wrote:

> > Oddly enough, yes.  But it doesn't explain why this code doesn't blow 
> > up every time it gets called, in its current form.
> 
> Well, it does :-)
> 
> dev_get_drvdata(_dev) -> NULL -> kfree(NULL)
> 
> We're just leaking memory. I guess a patch like below would be best:
> 
> diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c
> index 3828c2ec8623..4dc04253da61 100644
> --- a/drivers/usb/gadget/udc/net2280.c
> +++ b/drivers/usb/gadget/udc/net2280.c
> @@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
>  
>  /*-------------------------------------------------------------------------*/
>  
> -static void gadget_release(struct device *_dev)
> -{
> -	struct net2280	*dev = dev_get_drvdata(_dev);
> -
> -	kfree(dev);
> -}
> -
>  /* tear down the binding between this driver and the pci device */
>  
>  static void net2280_remove(struct pci_dev *pdev)
> @@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev)
>  	device_remove_file(&pdev->dev, &dev_attr_registers);
>  
>  	ep_info(dev, "unbind\n");
> +
> +	kfree(dev);
>  }
>  
>  /* wrap this driver around the specified device, but
> @@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (retval)
>  		goto done;
>  
> -	retval = usb_add_gadget_udc_release(&pdev->dev, &dev->gadget,
> -			gadget_release);
> +	retval = usb_add_gadget_udc(&pdev->dev, &dev->gadget);
>  	if (retval)
>  		goto done;
>  	return 0;

Maybe...  But I can't shake the feeling that Greg KH would strongly 
disagree.  Hasn't he said, many times in the past, that any dynamically 
allocated device structure _must_ have a real release routine?  
usb_udc_nop_release() doesn't qualify.

The issue is outstanding references to gadget.dev.  The driver core
does not guarantee that all references will have been dropped by the
time device_unregister() returns.  So what if some other part of the
kernel still has a reference to gadget.dev when we reach the end of
net2280_remove() and deallocate the private structure?

Unless you can be certain that there are no outstanding references when
the gadget is unregistered, this approach isn't safe.  (And even if you
can be certain, how can you know that future changes to the kernel
won't affect the situation?)

> > And it doesn't help with the fact that net2280_remove() continues to 
> > access the private data structure after calling usb_del_gadget_udc().  
> > Strictly speaking, that routine should do
> >
> > 	get_device(&dev->gadget.dev);
> >
> > at the start, with a corresponding put_device() at the end.
> >
> > There's another problem.  Suppose a call to 
> > usb_add_gadget_udc_release() fails.  At the end of that routine, the 
> > error pathway does put_device(&gadget->dev).  This will invoke the 
> > release callback, deallocating the private data structure without 
> > giving the caller (i.e., the UDC driver) a chance to clean up.
> 
> it won't deallocate anything :-) dev_set_drvdata() was never called,
> we will endup with kfree(NULL) which is safe and just silently returns.

But if you change gadget_release() to use the parent's drvdata field, 
like you proposed earlier, and fix up the refcounting in 
net2280_remove() so that the structure doesn't get deallocated too 
quickly, then this does become a real problem.

And it can affect other UDC drivers, not just net2280.

Alan Stern

[toc] | [prev] | [next] | [standalone]

#1621339

On Tue, Apr 11, 2017 at 10:12:01AM -0400, Alan Stern wrote:
> On Tue, 11 Apr 2017, Felipe Balbi wrote:
> 
> > > Oddly enough, yes.  But it doesn't explain why this code doesn't blow 
> > > up every time it gets called, in its current form.
> > 
> > Well, it does :-)
> > 
> > dev_get_drvdata(_dev) -> NULL -> kfree(NULL)
> > 
> > We're just leaking memory. I guess a patch like below would be best:
> > 
> > diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c
> > index 3828c2ec8623..4dc04253da61 100644
> > --- a/drivers/usb/gadget/udc/net2280.c
> > +++ b/drivers/usb/gadget/udc/net2280.c
> > @@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
> >  
> >  /*-------------------------------------------------------------------------*/
> >  
> > -static void gadget_release(struct device *_dev)
> > -{
> > -	struct net2280	*dev = dev_get_drvdata(_dev);
> > -
> > -	kfree(dev);
> > -}
> > -
> >  /* tear down the binding between this driver and the pci device */
> >  
> >  static void net2280_remove(struct pci_dev *pdev)
> > @@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev)
> >  	device_remove_file(&pdev->dev, &dev_attr_registers);
> >  
> >  	ep_info(dev, "unbind\n");
> > +
> > +	kfree(dev);
> >  }
> >  
> >  /* wrap this driver around the specified device, but
> > @@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >  	if (retval)
> >  		goto done;
> >  
> > -	retval = usb_add_gadget_udc_release(&pdev->dev, &dev->gadget,
> > -			gadget_release);
> > +	retval = usb_add_gadget_udc(&pdev->dev, &dev->gadget);
> >  	if (retval)
> >  		goto done;
> >  	return 0;
> 
> Maybe...  But I can't shake the feeling that Greg KH would strongly 
> disagree.  Hasn't he said, many times in the past, that any dynamically 
> allocated device structure _must_ have a real release routine?  
> usb_udc_nop_release() doesn't qualify.

Aw, I wanted to publically yell at someone like the kernel documentation
says I am allowed to do so if anyone does such a foolish thing :)

[toc] | [prev] | [next] | [standalone]

#1621824 — Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Multipart message — attachments visible in raw view] — view raw

Hi,

Greg KH <greg@kroah.com> writes:
> On Tue, Apr 11, 2017 at 10:12:01AM -0400, Alan Stern wrote:
>> On Tue, 11 Apr 2017, Felipe Balbi wrote:
>> 
>> > > Oddly enough, yes.  But it doesn't explain why this code doesn't blow 
>> > > up every time it gets called, in its current form.
>> > 
>> > Well, it does :-)
>> > 
>> > dev_get_drvdata(_dev) -> NULL -> kfree(NULL)
>> > 
>> > We're just leaking memory. I guess a patch like below would be best:
>> > 
>> > diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c
>> > index 3828c2ec8623..4dc04253da61 100644
>> > --- a/drivers/usb/gadget/udc/net2280.c
>> > +++ b/drivers/usb/gadget/udc/net2280.c
>> > @@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
>> >  
>> >  /*-------------------------------------------------------------------------*/
>> >  
>> > -static void gadget_release(struct device *_dev)
>> > -{
>> > -	struct net2280	*dev = dev_get_drvdata(_dev);
>> > -
>> > -	kfree(dev);
>> > -}
>> > -
>> >  /* tear down the binding between this driver and the pci device */
>> >  
>> >  static void net2280_remove(struct pci_dev *pdev)
>> > @@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev)
>> >  	device_remove_file(&pdev->dev, &dev_attr_registers);
>> >  
>> >  	ep_info(dev, "unbind\n");
>> > +
>> > +	kfree(dev);
>> >  }
>> >  
>> >  /* wrap this driver around the specified device, but
>> > @@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> >  	if (retval)
>> >  		goto done;
>> >  
>> > -	retval = usb_add_gadget_udc_release(&pdev->dev, &dev->gadget,
>> > -			gadget_release);
>> > +	retval = usb_add_gadget_udc(&pdev->dev, &dev->gadget);
>> >  	if (retval)
>> >  		goto done;
>> >  	return 0;
>> 
>> Maybe...  But I can't shake the feeling that Greg KH would strongly 
>> disagree.  Hasn't he said, many times in the past, that any dynamically 
>> allocated device structure _must_ have a real release routine?  
>> usb_udc_nop_release() doesn't qualify.
>
> Aw, I wanted to publically yell at someone like the kernel documentation
> says I am allowed to do so if anyone does such a foolish thing :)

heh, except that we're not dynamically allocating struct device at all
:-) Here's what we have for most UDCs (net2280.c included):

	struct my_udc {
        	struct gadget gadget;
                [...]
	};

	probe()
        {
        	struct my_udc *u;

		u = kzalloc(sizeof(*u), GFP_KERNEL);
                [...]
		return 0;
	}

Now, if this kzalloc() would be replaced with devm_kzalloc() wouldn't
this result on a functionally equivalent execution to the patch I
proposed above?

Iff we change struct gadget to contain a struct device *dev instead of a
struct device dev, then sure, we will need to cope with proper
->release() implementations.

As it is, it brings nothing to the table, IMO.

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1621844

On Wed, Apr 12, 2017 at 09:01:44AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Greg KH <greg@kroah.com> writes:
> > On Tue, Apr 11, 2017 at 10:12:01AM -0400, Alan Stern wrote:
> >> On Tue, 11 Apr 2017, Felipe Balbi wrote:
> >> 
> >> > > Oddly enough, yes.  But it doesn't explain why this code doesn't blow 
> >> > > up every time it gets called, in its current form.
> >> > 
> >> > Well, it does :-)
> >> > 
> >> > dev_get_drvdata(_dev) -> NULL -> kfree(NULL)
> >> > 
> >> > We're just leaking memory. I guess a patch like below would be best:
> >> > 
> >> > diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c
> >> > index 3828c2ec8623..4dc04253da61 100644
> >> > --- a/drivers/usb/gadget/udc/net2280.c
> >> > +++ b/drivers/usb/gadget/udc/net2280.c
> >> > @@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
> >> >  
> >> >  /*-------------------------------------------------------------------------*/
> >> >  
> >> > -static void gadget_release(struct device *_dev)
> >> > -{
> >> > -	struct net2280	*dev = dev_get_drvdata(_dev);
> >> > -
> >> > -	kfree(dev);
> >> > -}
> >> > -
> >> >  /* tear down the binding between this driver and the pci device */
> >> >  
> >> >  static void net2280_remove(struct pci_dev *pdev)
> >> > @@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev)
> >> >  	device_remove_file(&pdev->dev, &dev_attr_registers);
> >> >  
> >> >  	ep_info(dev, "unbind\n");
> >> > +
> >> > +	kfree(dev);
> >> >  }
> >> >  
> >> >  /* wrap this driver around the specified device, but
> >> > @@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >> >  	if (retval)
> >> >  		goto done;
> >> >  
> >> > -	retval = usb_add_gadget_udc_release(&pdev->dev, &dev->gadget,
> >> > -			gadget_release);
> >> > +	retval = usb_add_gadget_udc(&pdev->dev, &dev->gadget);
> >> >  	if (retval)
> >> >  		goto done;
> >> >  	return 0;
> >> 
> >> Maybe...  But I can't shake the feeling that Greg KH would strongly 
> >> disagree.  Hasn't he said, many times in the past, that any dynamically 
> >> allocated device structure _must_ have a real release routine?  
> >> usb_udc_nop_release() doesn't qualify.
> >
> > Aw, I wanted to publically yell at someone like the kernel documentation
> > says I am allowed to do so if anyone does such a foolish thing :)
> 
> heh, except that we're not dynamically allocating struct device at all
> :-)

Please don't say that, that's even worse :(

> Here's what we have for most UDCs (net2280.c included):
> 
> 	struct my_udc {
>         	struct gadget gadget;
>                 [...]
> 	};
> 
> 	probe()
>         {
>         	struct my_udc *u;
> 
> 		u = kzalloc(sizeof(*u), GFP_KERNEL);
>                 [...]
> 		return 0;
> 	}
> 
> Now, if this kzalloc() would be replaced with devm_kzalloc() wouldn't
> this result on a functionally equivalent execution to the patch I
> proposed above?
> 
> Iff we change struct gadget to contain a struct device *dev instead of a
> struct device dev, then sure, we will need to cope with proper
> ->release() implementations.
> 
> As it is, it brings nothing to the table, IMO.

You always have to have a release function for a kobject, no matter
where it is, as it is being reference counted.  To not do so, is a huge
indication of a problem in the design.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1621878 — Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Multipart message — attachments visible in raw view] — view raw

Hi,

Greg KH <greg@kroah.com> writes:
>> Here's what we have for most UDCs (net2280.c included):
>> 
>> 	struct my_udc {
>>         	struct gadget gadget;
>>                 [...]
>> 	};
>> 
>> 	probe()
>>         {
>>         	struct my_udc *u;
>> 
>> 		u = kzalloc(sizeof(*u), GFP_KERNEL);
>>                 [...]
>> 		return 0;
>> 	}
>> 
>> Now, if this kzalloc() would be replaced with devm_kzalloc() wouldn't
>> this result on a functionally equivalent execution to the patch I
>> proposed above?
>> 
>> Iff we change struct gadget to contain a struct device *dev instead of a
>> struct device dev, then sure, we will need to cope with proper
>> ->release() implementations.
>> 
>> As it is, it brings nothing to the table, IMO.
>
> You always have to have a release function for a kobject, no matter
> where it is, as it is being reference counted.  To not do so, is a huge
> indication of a problem in the design.

okay, this goes all the way back to when Dave B wrote the API, it has
always had empty ->release() functions (not all UDCs, though). I'll make
sure to review all of this for v4.13.

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1622183

On Wed, 12 Apr 2017, Felipe Balbi wrote:

> >> Maybe...  But I can't shake the feeling that Greg KH would strongly 
> >> disagree.  Hasn't he said, many times in the past, that any dynamically 
> >> allocated device structure _must_ have a real release routine?  
> >> usb_udc_nop_release() doesn't qualify.
> >
> > Aw, I wanted to publically yell at someone like the kernel documentation
> > says I am allowed to do so if anyone does such a foolish thing :)
> 
> heh, except that we're not dynamically allocating struct device at all
> :-) Here's what we have for most UDCs (net2280.c included):
> 
> 	struct my_udc {
>         	struct gadget gadget;
>                 [...]
> 	};
> 
> 	probe()
>         {
>         	struct my_udc *u;
> 
> 		u = kzalloc(sizeof(*u), GFP_KERNEL);
>                 [...]
> 		return 0;
> 	}

Allow me to point out that the struct device is embedded inside the
struct gadget (actually struct usb_gadget) embedded inside the struct
my_udc, which _is_ dynamically allocated.  Therefore the struct device
is located in dynamically allocated memory.

> Now, if this kzalloc() would be replaced with devm_kzalloc() wouldn't
> this result on a functionally equivalent execution to the patch I
> proposed above?

It would, and it would be equally wrong.

Alan Stern

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web