| Message ID | 20180701090553.7776-2-miguel@det.uvigo.gal (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On So, 2018-07-01 at 11:05 +0200, Miguel Rodríguez Pérez wrote: > Remove some unneded varibles to make the code easier to read > and, replace the generic usb_control_msg function for the > more specific usbnet_write_cmd. > > Signed-off-by: Miguel Rodríguez Pérez <miguel@det.uvigo.gal> No, sorry, but this is not good. The reason is a bit subtle. Drivers need to reset the filters when handling post_reset() [ and reset_resume() ] usbnet_write_cmd() falls back to kmemdup() with GFP_KERNEL. Usbnet is a framework with class drivers and some of the devices we drive have a storage interface. Thence we are on the block error handling path here. The simplest solution is to leave out this patch in the sequence. Regards Oliver NACKED-BY: Oliver Neukum <oneukum@suse.com> > --- > drivers/net/usb/cdc_ether.c | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c > index 178b956501a7..815ed0dc18fe 100644 > --- a/drivers/net/usb/cdc_ether.c > +++ b/drivers/net/usb/cdc_ether.c > @@ -77,9 +77,7 @@ static const u8 mbm_guid[16] = { > > static void usbnet_cdc_update_filter(struct usbnet *dev) > { > - struct cdc_state *info = (void *) &dev->data; > - struct usb_interface *intf = info->control; > - struct net_device *net = dev->net; > + struct net_device *net = dev->net; > > u16 cdc_filter = USB_CDC_PACKET_TYPE_DIRECTED > | USB_CDC_PACKET_TYPE_BROADCAST; > @@ -93,16 +91,13 @@ static void usbnet_cdc_update_filter(struct usbnet *dev) > if (!netdev_mc_empty(net) || (net->flags & IFF_ALLMULTI)) > cdc_filter |= USB_CDC_PACKET_TYPE_ALL_MULTICAST; > > - usb_control_msg(dev->udev, > - usb_sndctrlpipe(dev->udev, 0), > + usbnet_write_cmd(dev, > USB_CDC_SET_ETHERNET_PACKET_FILTER, > - USB_TYPE_CLASS | USB_RECIP_INTERFACE, > + USB_TYPE_CLASS | USB_DIR_OUT | USB_RECIP_INTERFACE, > cdc_filter, > - intf->cur_altsetting->desc.bInterfaceNumber, > + dev->intf->cur_altsetting->desc.bInterfaceNumber, > NULL, > - 0, > - USB_CTRL_SET_TIMEOUT > - ); > + 0); > } > > /* probes control interface, claims data interface, collects the bulk -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I get a panic if I remove this patch, because intf comes NULL for cdc_ncm devices. I'll send an updated patch that solves this issue while still using usb_control_msg. On 02/07/18 10:25, Oliver Neukum wrote: > On So, 2018-07-01 at 11:05 +0200, Miguel Rodríguez Pérez wrote: >> Remove some unneded varibles to make the code easier to read >> and, replace the generic usb_control_msg function for the >> more specific usbnet_write_cmd. >> >> Signed-off-by: Miguel Rodríguez Pérez <miguel@det.uvigo.gal> > > No, > > sorry, but this is not good. The reason is a bit subtle. > Drivers need to reset the filters when handling post_reset() > [ and reset_resume() ] usbnet_write_cmd() falls back to > kmemdup() with GFP_KERNEL. Usbnet is a framework with class > drivers and some of the devices we drive have a storage > interface. Thence we are on the block error handling path here. > > The simplest solution is to leave out this patch in the sequence. > > Regards > Oliver > > > NACKED-BY: Oliver Neukum <oneukum@suse.com> > > >> --- >> drivers/net/usb/cdc_ether.c | 15 +++++---------- >> 1 file changed, 5 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c >> index 178b956501a7..815ed0dc18fe 100644 >> --- a/drivers/net/usb/cdc_ether.c >> +++ b/drivers/net/usb/cdc_ether.c >> @@ -77,9 +77,7 @@ static const u8 mbm_guid[16] = { >> >> static void usbnet_cdc_update_filter(struct usbnet *dev) >> { >> - struct cdc_state *info = (void *) &dev->data; >> - struct usb_interface *intf = info->control; >> - struct net_device *net = dev->net; >> + struct net_device *net = dev->net; >> >> u16 cdc_filter = USB_CDC_PACKET_TYPE_DIRECTED >> | USB_CDC_PACKET_TYPE_BROADCAST; >> @@ -93,16 +91,13 @@ static void usbnet_cdc_update_filter(struct usbnet *dev) >> if (!netdev_mc_empty(net) || (net->flags & IFF_ALLMULTI)) >> cdc_filter |= USB_CDC_PACKET_TYPE_ALL_MULTICAST; >> >> - usb_control_msg(dev->udev, >> - usb_sndctrlpipe(dev->udev, 0), >> + usbnet_write_cmd(dev, >> USB_CDC_SET_ETHERNET_PACKET_FILTER, >> - USB_TYPE_CLASS | USB_RECIP_INTERFACE, >> + USB_TYPE_CLASS | USB_DIR_OUT | USB_RECIP_INTERFACE, >> cdc_filter, >> - intf->cur_altsetting->desc.bInterfaceNumber, >> + dev->intf->cur_altsetting->desc.bInterfaceNumber, >> NULL, >> - 0, >> - USB_CTRL_SET_TIMEOUT >> - ); >> + 0); >> } >> >> /* probes control interface, claims data interface, collects the bulk >
Oliver Neukum <oneukum@suse.com> writes: > On So, 2018-07-01 at 11:05 +0200, Miguel Rodríguez Pérez wrote: >> Remove some unneded varibles to make the code easier to read >> and, replace the generic usb_control_msg function for the >> more specific usbnet_write_cmd. >> >> Signed-off-by: Miguel Rodríguez Pérez <miguel@det.uvigo.gal> > > No, > > sorry, but this is not good. The reason is a bit subtle. > Drivers need to reset the filters when handling post_reset() > [ and reset_resume() ] usbnet_write_cmd() falls back to > kmemdup() with GFP_KERNEL. Usbnet is a framework with class > drivers and some of the devices we drive have a storage > interface. Thence we are on the block error handling path here. Right. I knew there had to be some reason this was left out when the rest of these were converted. But I don't think the reason is valid... usbnet_write_cmd() will never call kmemdup when data is NULL. There is of course also little advantage in using usbnet_write_cmd when we don't need to allocate a buffer. But I'd still prefer to see it for consistency, power management and debug logging. Or if it is left as-is: Maybe add a comment so that I don't ask the same stupid questions in 3 weeks time? :-) My memory is los^Husy... > The simplest solution is to leave out this patch in the sequence. As Miguel noted: That won't work. The switch from dev->data->control to dev->intf is necessary to make this function reusable by other minidrivers. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hey, I've encountered that same problem a few days ago, found this thread and these (unmerged) patches, "ported" them to a more current version of the kernel (wasn't much work, let's be honest), and I was wondering if it would be possible to get them merged, because this is still a problem with cdc_ncm. Here's the three "up to date" patches (based on 5.8-rc5), they work insofar as I'm running with them, the bug isn't there anymore (I get ethernet multicast packets correctly), and there's no obvious bug. I'm not a dev though, so I have no idea if these are formatted correctly, if the patch separation is correct, or anything like that, I just think it would be great if this could be merged into mainline! On Sun, 2018-07-01 at 11:05 +0200, Miguel Rodríguez Pérez wrote: > Remove some unneded varibles to make the code easier to read > and, replace the generic usb_control_msg function for the > more specific usbnet_write_cmd. > > Signed-off-by: Miguel Rodríguez Pérez <miguel@det.uvigo.gal> > NACKED-BY: Oliver Neukum <oneukum@suse.com> > --- > drivers/net/usb/cdc_ether.c | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/usb/cdc_ether.c > b/drivers/net/usb/cdc_ether.c > index 178b956501a7..815ed0dc18fe 100644 > --- a/drivers/net/usb/cdc_ether.c > +++ b/drivers/net/usb/cdc_ether.c > @@ -77,9 +77,7 @@ static const u8 mbm_guid[16] = { > > static void usbnet_cdc_update_filter(struct usbnet *dev) > { > - struct cdc_state *info = (void *) &dev->data; > - struct usb_interface *intf = info->control; > - struct net_device *net = dev->net; > + struct net_device *net = dev->net; > > u16 cdc_filter = USB_CDC_PACKET_TYPE_DIRECTED > | USB_CDC_PACKET_TYPE_BROADCAST; > @@ -93,16 +91,13 @@ static void usbnet_cdc_update_filter(struct > usbnet *dev) > if (!netdev_mc_empty(net) || (net->flags & IFF_ALLMULTI)) > cdc_filter |= USB_CDC_PACKET_TYPE_ALL_MULTICAST; > > - usb_control_msg(dev->udev, > - usb_sndctrlpipe(dev->udev, 0), > + usbnet_write_cmd(dev, > USB_CDC_SET_ETHERNET_PACKET_FILTER, > - USB_TYPE_CLASS | USB_RECIP_INTERFACE, > + USB_TYPE_CLASS | USB_DIR_OUT | > USB_RECIP_INTERFACE, > cdc_filter, > - intf->cur_altsetting->desc.bInterfaceNumber, > + dev->intf->cur_altsetting- > >desc.bInterfaceNumber, > NULL, > - 0, > - USB_CTRL_SET_TIMEOUT > - ); > + 0); > } > > /* probes control interface, claims data interface, collects the > bulk
On Mon, Jul 13, 2020 at 04:43:11PM -0400, Wxcafé wrote: > Hey, > > I've encountered that same problem a few days ago, found this thread > and these (unmerged) patches, "ported" them to a more current version > of the kernel (wasn't much work, let's be honest), and I was wondering > if it would be possible to get them merged, because this is still a > problem with cdc_ncm. > > Here's the three "up to date" patches (based on 5.8-rc5), they work > insofar as I'm running with them, the bug isn't there anymore (I get > ethernet multicast packets correctly), and there's no obvious bug. I'm > not a dev though, so I have no idea if these are formatted correctly, > if the patch separation is correct, or anything like that, I just think > it would be great if this could be merged into mainline! You need to submit them in a format they can be applied in, as well as taking care of the issues that caused Oliver to not agree with them. thanks, greg k-h
Hi Greg, Thanks for your feedback! I'm not sure what you mean by "a format they can be applied in", I'm guessing as three emails, with a single patch each? As for the issues that you mention, I'm guessing you mean applying v4 of Miguel's patches as found here ( https://patchwork.kernel.org/patch/10501163/, https://patchwork.kernel.org/patch/10507009/) rather than v3 as I did here (I didn't notice there was a v4 fixing those issues, and reading the previous messages in this thread I assumed they weren't considered blocking. Or is there a problem left with v4 of the patches? There doesn't seem to be any replies to the messages from 2 years ago) Sorry for all my questions, I'm totally new to all this process... Cheers On Tue, 2020-07-14 at 08:06 +0200, Greg KH wrote: > On Mon, Jul 13, 2020 at 04:43:11PM -0400, Wxcafé wrote: > > Hey, > > > > I've encountered that same problem a few days ago, found this > > thread > > and these (unmerged) patches, "ported" them to a more current > > version > > of the kernel (wasn't much work, let's be honest), and I was > > wondering > > if it would be possible to get them merged, because this is still a > > problem with cdc_ncm. > > > > Here's the three "up to date" patches (based on 5.8-rc5), they work > > insofar as I'm running with them, the bug isn't there anymore (I > > get > > ethernet multicast packets correctly), and there's no obvious bug. > > I'm > > not a dev though, so I have no idea if these are formatted > > correctly, > > if the patch separation is correct, or anything like that, I just > > think > > it would be great if this could be merged into mainline! > > You need to submit them in a format they can be applied in, as well > as > taking care of the issues that caused Oliver to not agree with them. > > thanks, > > greg k-h
On Tue, Jul 14, 2020 at 11:13:18AM -0400, Wxcafé wrote: > Hi Greg, > > Thanks for your feedback! I'm not sure what you mean by "a format they > can be applied in", I'm guessing as three emails, with a single patch > each? We have loads of documentation about how to do this in the kernel tree, you might want to read up on it starting at Documentation/process/ thanks! greg k-h
diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c index 178b956501a7..815ed0dc18fe 100644 --- a/drivers/net/usb/cdc_ether.c +++ b/drivers/net/usb/cdc_ether.c @@ -77,9 +77,7 @@ static const u8 mbm_guid[16] = { static void usbnet_cdc_update_filter(struct usbnet *dev) { - struct cdc_state *info = (void *) &dev->data; - struct usb_interface *intf = info->control; - struct net_device *net = dev->net; + struct net_device *net = dev->net; u16 cdc_filter = USB_CDC_PACKET_TYPE_DIRECTED | USB_CDC_PACKET_TYPE_BROADCAST; @@ -93,16 +91,13 @@ static void usbnet_cdc_update_filter(struct usbnet *dev) if (!netdev_mc_empty(net) || (net->flags & IFF_ALLMULTI)) cdc_filter |= USB_CDC_PACKET_TYPE_ALL_MULTICAST; - usb_control_msg(dev->udev, - usb_sndctrlpipe(dev->udev, 0), + usbnet_write_cmd(dev, USB_CDC_SET_ETHERNET_PACKET_FILTER, - USB_TYPE_CLASS | USB_RECIP_INTERFACE, + USB_TYPE_CLASS | USB_DIR_OUT | USB_RECIP_INTERFACE, cdc_filter, - intf->cur_altsetting->desc.bInterfaceNumber, + dev->intf->cur_altsetting->desc.bInterfaceNumber, NULL, - 0, - USB_CTRL_SET_TIMEOUT - ); + 0); } /* probes control interface, claims data interface, collects the bulk
Remove some unneded varibles to make the code easier to read and, replace the generic usb_control_msg function for the more specific usbnet_write_cmd. Signed-off-by: Miguel Rodríguez Pérez <miguel@det.uvigo.gal> --- drivers/net/usb/cdc_ether.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)