| Message ID | 20180717120411.GB28592@kroah.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 17 Jul 2018, Greg KH wrote: > > From: Sudip Mukherjee <sudipm.mukherjee@gmail.com> > > Date: Tue, 10 Jul 2018 09:50:00 +0100 > > Subject: [PATCH] hacky solution to mem-corruption > > > > Signed-off-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com> > > --- > > drivers/usb/core/message.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c > > index 7cd4ec33dbf4..7fdf7a27611d 100644 > > --- a/drivers/usb/core/message.c > > +++ b/drivers/usb/core/message.c > > @@ -1398,7 +1398,8 @@ int usb_set_interface(struct usb_device *dev, int interface, int alternate) > > remove_intf_ep_devs(iface); > > usb_remove_sysfs_intf_files(iface); > > } > > - usb_disable_interface(dev, iface, true); > > + if (!(iface->cur_altsetting && alt)) > > + usb_disable_interface(dev, iface, true); > > > > This feels like a "correct" patch anyway, why would a driver keep > calling set_interface to an interface that it was already set to? > > But can't we check for this higher up in the function? This hack will > just not disable an interface but it will do all of the other stuff > being asked for. Does the patch below also solve this for you? It's > not a good solution of course, but it might work around the problem a > bit better. > > thanks, > > greg k-h > > diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c > index 1a15392326fc..0f718f1a1ca3 100644 > --- a/drivers/usb/core/message.c > +++ b/drivers/usb/core/message.c > @@ -1376,6 +1376,14 @@ int usb_set_interface(struct usb_device *dev, int interface, int alternate) > return -EINVAL; > } > > + if (iface->cur_altsetting == alt) { > + /* > + * foolish bluetooth stack, don't try to set a setting you are > + * already set to... > + */ > + return 0; > + } > + > /* Make sure we have enough bandwidth for this alternate interface. > * Remove the current alt setting and add the new alt setting. > */ No, neither of these is right. It's possible to use usb_set_interface() as a kind of "soft" reset. Even when the new altsetting is specified to be the same as the current one, we still have to tell the lower-layer drivers and hardware about it. Alan Stern -- 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
On Tue, Jul 17, 2018 at 10:31:38AM -0400, Alan Stern wrote: > On Tue, 17 Jul 2018, Greg KH wrote: > > > > From: Sudip Mukherjee <sudipm.mukherjee@gmail.com> > > > Date: Tue, 10 Jul 2018 09:50:00 +0100 > > > Subject: [PATCH] hacky solution to mem-corruption > > > > > > Signed-off-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com> > > > --- > > > drivers/usb/core/message.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c > > > index 7cd4ec33dbf4..7fdf7a27611d 100644 > > > --- a/drivers/usb/core/message.c > > > +++ b/drivers/usb/core/message.c > > > @@ -1398,7 +1398,8 @@ int usb_set_interface(struct usb_device *dev, int interface, int alternate) > > > remove_intf_ep_devs(iface); > > > usb_remove_sysfs_intf_files(iface); > > > } > > > - usb_disable_interface(dev, iface, true); > > > + if (!(iface->cur_altsetting && alt)) > > > + usb_disable_interface(dev, iface, true); > > > > > > > > This feels like a "correct" patch anyway, why would a driver keep > > calling set_interface to an interface that it was already set to? > > > > But can't we check for this higher up in the function? This hack will > > just not disable an interface but it will do all of the other stuff > > being asked for. Does the patch below also solve this for you? It's > > not a good solution of course, but it might work around the problem a > > bit better. > > > > thanks, > > > > greg k-h > > > > diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c > > index 1a15392326fc..0f718f1a1ca3 100644 > > --- a/drivers/usb/core/message.c > > +++ b/drivers/usb/core/message.c > > @@ -1376,6 +1376,14 @@ int usb_set_interface(struct usb_device *dev, int interface, int alternate) > > return -EINVAL; > > } > > > > + if (iface->cur_altsetting == alt) { > > + /* > > + * foolish bluetooth stack, don't try to set a setting you are > > + * already set to... > > + */ > > + return 0; > > + } > > + > > /* Make sure we have enough bandwidth for this alternate interface. > > * Remove the current alt setting and add the new alt setting. > > */ > > No, neither of these is right. It's possible to use > usb_set_interface() as a kind of "soft" reset. Even when the new > altsetting is specified to be the same as the current one, we still > have to tell the lower-layer drivers and hardware about it. You are right, it's a hacky soft reset, I was just trying to figure out what the bluetooth driver was trying to do. I wouldn't expect it to be calling that function a lot, but I guess it does :( thanks, greg k-h -- 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
On Tue, Jul 17, 2018 at 05:52:59PM +0200, Greg KH wrote: > On Tue, Jul 17, 2018 at 10:31:38AM -0400, Alan Stern wrote: > > On Tue, 17 Jul 2018, Greg KH wrote: > > > > > > From: Sudip Mukherjee <sudipm.mukherjee@gmail.com> > > > > Date: Tue, 10 Jul 2018 09:50:00 +0100 > > > > Subject: [PATCH] hacky solution to mem-corruption > > > > > > > > Signed-off-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com> > > > > --- <snip> > > > > No, neither of these is right. It's possible to use > > usb_set_interface() as a kind of "soft" reset. Even when the new > > altsetting is specified to be the same as the current one, we still > > have to tell the lower-layer drivers and hardware about it. > > You are right, it's a hacky soft reset, I was just trying to figure out > what the bluetooth driver was trying to do. I wouldn't expect it to be > calling that function a lot, but I guess it does :( usb_set_interface() is being called two times from bluetooth event. But I am now adding more debugs to see why your patch did not work. -- Regards Sudip -- 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
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c index 1a15392326fc..0f718f1a1ca3 100644 --- a/drivers/usb/core/message.c +++ b/drivers/usb/core/message.c @@ -1376,6 +1376,14 @@ int usb_set_interface(struct usb_device *dev, int interface, int alternate) return -EINVAL; } + if (iface->cur_altsetting == alt) { + /* + * foolish bluetooth stack, don't try to set a setting you are + * already set to... + */ + return 0; + } + /* Make sure we have enough bandwidth for this alternate interface. * Remove the current alt setting and add the new alt setting. */