Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On Thu, Jun 21, 2018 at 12:32 AM, Rafael J. Wysocki wrote: > On Wed, Jun 20, 2018 at 5:46 PM, Johan Hovold wrote: >> On Wed, Jun 20, 2018 at 02:54:10PM +0200, Rafael J. Wysocki wrote: >>> On Wednesday, June 20, 2018 2:23:46 PM CEST Johan Hovold wrote: >>> > On Wed, Jun 20, 2018 at 02:16:59AM -0700, Tony Lindgren wrote: >>> > > Hi, >>> > > >>> > > Adding Rafael and linux-pm to Cc as well. >>> > > >>> > > * Felipe Balbi [180619 01:23]: >>> > > > This is a direct consequence of not paying attention to the order of >>> > > > things. If driver were to assume that pm_domain->activate() would do >>> > > > the >>> > > > right thing for the device -- meaning that probe would run with an >>> > > > active device --, then we wouldn't need that pm_runtime_get() call on >>> > > > probe at all. Rather we would follow the sequence: >>> > > > >>> > > > pm_runtime_forbid() >>> > > > pm_runtime_set_active() >>> > > > pm_runtime_enable() >>> > > > >>> > > > /* do your probe routine */ >>> > > > >>> > > > pm_runtime_put_noidle() >>> > > > >>> > > > Then you remove you would need to call pm_runtime_get_noresume() to >>> > > > balance out the pm_runtime_put_noidle() there. >>> > >>> > > > (If you need to know why the pm_runtime_put_noidle(), remember that >>> > > > pm_runtime_set_active() increments the usage counter, so >>> > > > pm_runtime_put_noidle is basically allowing pm_runtime to happen as >>> > > > soon >>> > > > as userspace writes "auto" to /sys//power/control) >>> > >>> > That's not correct; pm_runtime_set_active() only increments the usage >>> > counter of a parent (under some circumstances), so unless you have bus >>> > code incrementing the usage counter before probe, the above >>> > pm_runtime_put_noidle() would actually introduce an imbalance. >>> >>> No, it wouldn't. It balances the incrementation in pm_runtime_forbid(). >> >> Right, but even if you take the whole sequence, which included >> pm_runtime_forbid(), consider what happens when pm_runtime_allow() is >> later called through sysfs (see below). >> >>> > And note that that's also the case even if you meant to say that >>> > *pm_runtime_forbid()* increments the usage counter (which it does). >>> >>> Why is it? >>> >>> Surely, after >>> >>> pm_runtime_forbid(dev); >>> pm_runtime_put_noidle(dev); >>> >>> the runtime PM usage counter of dev will be the same as before, won't it? >> >> Sure, but the imbalance, or rather inconsistent state, has already been >> introduced. >> >> Consider the following sequence of events: >> >> usage count >> 0 >> probe() >> pm_runtime_forbid() 1 >> pm_runtime_set_active() >> pm_runtime_enable() >> pm_runtime_put_noidle() 0 >> >> Here nothing is preventing the device from runtime suspending, despite >> runtime PM being forbidden. In fact, it will typically be suspended due >> to the pm_request_idle() in driver_probe_device(). If later we have: >> >> echo auto > power/control >> pm_runtime_allow() -1 > > OK, you have a point. > > After calling pm_runtime_forbid() the driver should allow user space > to unblock runtime PM for the device - or call pm_runtime_allow() > itself. The confusion regarding the pm_runtime_put_noidle() at the end may come from the special requirement of the PCI bus type as per the comment in local_pci_probe(). -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On Wed, Jun 20, 2018 at 5:46 PM, Johan Hovold wrote: > On Wed, Jun 20, 2018 at 02:54:10PM +0200, Rafael J. Wysocki wrote: >> On Wednesday, June 20, 2018 2:23:46 PM CEST Johan Hovold wrote: >> > On Wed, Jun 20, 2018 at 02:16:59AM -0700, Tony Lindgren wrote: >> > > Hi, >> > > >> > > Adding Rafael and linux-pm to Cc as well. >> > > >> > > * Felipe Balbi [180619 01:23]: >> > > > This is a direct consequence of not paying attention to the order of >> > > > things. If driver were to assume that pm_domain->activate() would do >> > > > the >> > > > right thing for the device -- meaning that probe would run with an >> > > > active device --, then we wouldn't need that pm_runtime_get() call on >> > > > probe at all. Rather we would follow the sequence: >> > > > >> > > > pm_runtime_forbid() >> > > > pm_runtime_set_active() >> > > > pm_runtime_enable() >> > > > >> > > > /* do your probe routine */ >> > > > >> > > > pm_runtime_put_noidle() >> > > > >> > > > Then you remove you would need to call pm_runtime_get_noresume() to >> > > > balance out the pm_runtime_put_noidle() there. >> > >> > > > (If you need to know why the pm_runtime_put_noidle(), remember that >> > > > pm_runtime_set_active() increments the usage counter, so >> > > > pm_runtime_put_noidle is basically allowing pm_runtime to happen as >> > > > soon >> > > > as userspace writes "auto" to /sys//power/control) >> > >> > That's not correct; pm_runtime_set_active() only increments the usage >> > counter of a parent (under some circumstances), so unless you have bus >> > code incrementing the usage counter before probe, the above >> > pm_runtime_put_noidle() would actually introduce an imbalance. >> >> No, it wouldn't. It balances the incrementation in pm_runtime_forbid(). > > Right, but even if you take the whole sequence, which included > pm_runtime_forbid(), consider what happens when pm_runtime_allow() is > later called through sysfs (see below). > >> > And note that that's also the case even if you meant to say that >> > *pm_runtime_forbid()* increments the usage counter (which it does). >> >> Why is it? >> >> Surely, after >> >> pm_runtime_forbid(dev); >> pm_runtime_put_noidle(dev); >> >> the runtime PM usage counter of dev will be the same as before, won't it? > > Sure, but the imbalance, or rather inconsistent state, has already been > introduced. > > Consider the following sequence of events: > > usage count > 0 > probe() > pm_runtime_forbid() 1 > pm_runtime_set_active() > pm_runtime_enable() > pm_runtime_put_noidle() 0 > > Here nothing is preventing the device from runtime suspending, despite > runtime PM being forbidden. In fact, it will typically be suspended due > to the pm_request_idle() in driver_probe_device(). If later we have: > > echo auto > power/control > pm_runtime_allow() -1 OK, you have a point. After calling pm_runtime_forbid() the driver should allow user space to unblock runtime PM for the device - or call pm_runtime_allow() itself. [cut] > > And if runtime pm is later again forbidden: > > echo on > power/control > pm_runtime_forbid() 0 > > then the device will not be resumed. But I don't quite see why that will be the case. rpm_resume() will still be called and it doesn't look at the usage counter. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/9] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()
On Wed, 20 Jun 2018 17:47:01 +0200,
Sebastian Andrzej Siewior wrote:
>
> On 2018-06-20 17:38:44 [+0200], To Takashi Iwai wrote:
> > okay, as you wish:
>
> I'm sorry, I compiled one patch and send the other. Here is the fixed
> one.
Well, you seem to have forgotten to update the changelog...
Don't need to rush, it's a change for 4.19.
thanks,
Takashi
>
> > > thanks,
> > >
> > > Takashi
> >
> --- >8
> Subject: [PATCH 8/9 v4] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()
>
> Using usb_fill_int_urb() helps to find code which initializes an
> URB. A grep for members of the struct (like ->complete) reveal lots
> of other things, too.
>
> The "&& !(*purb)->transfer_buffer" check has been removed because the
> URB has been freshly allocated a few lines above so ->transfer_buffer
> has to be NULL here.
> The `dev' and `transfer_size' assignments have been moved from
> usX2Y_urbs_start() to usX2Y_urbs_allocate() because they don't change
> overtime.
> The multiplication via check_mul_overflow() has been extracted from
> kmalloc_array() to avoid two multiplication (one with overflow check and
> one without for the length argument). This requires to change the type
> `maxpacksize' to int so there is only one type involved in the
> multiplication.
>
> Cc: Jaroslav Kysela
> Cc: Takashi Iwai
> Signed-off-by: Sebastian Andrzej Siewior
> ---
> sound/usb/usx2y/usbusx2yaudio.c | 23 +++
> 1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
> index 2b833054e3b0..de3a21444db3 100644
> --- a/sound/usb/usx2y/usbusx2yaudio.c
> +++ b/sound/usb/usx2y/usbusx2yaudio.c
> @@ -425,6 +425,9 @@ static int usX2Y_urbs_allocate(struct snd_usX2Y_substream
> *subs)
> /* allocate and initialize data urbs */
> for (i = 0; i < NRURBS; i++) {
> struct urb **purb = subs->urb + i;
> + void *buf = NULL;
> + int len = 0;
> +
> if (*purb) {
> usb_kill_urb(*purb);
> continue;
> @@ -434,22 +437,20 @@ static int usX2Y_urbs_allocate(struct
> snd_usX2Y_substream *subs)
> usX2Y_urbs_release(subs);
> return -ENOMEM;
> }
> - if (!is_playback && !(*purb)->transfer_buffer) {
> +
> + if (!is_playback) {
> /* allocate a capture buffer per urb */
> - (*purb)->transfer_buffer =
> - kmalloc_array(subs->maxpacksize,
> - nr_of_packs(), GFP_KERNEL);
> - if (NULL == (*purb)->transfer_buffer) {
> + buf = kmalloc_array(subs->maxpacksize, nr_of_packs(),
> + GFP_KERNEL);
> + if (!buf) {
> usX2Y_urbs_release(subs);
> return -ENOMEM;
> }
> + len = subs->maxpacksize * nr_of_packs();
> }
> - (*purb)->dev = dev;
> - (*purb)->pipe = pipe;
> + usb_fill_int_urb(*purb, dev, pipe, buf, len,
> + i_usX2Y_subs_startup, subs, 1);
> (*purb)->number_of_packets = nr_of_packs();
> - (*purb)->context = subs;
> - (*purb)->interval = 1;
> - (*purb)->complete = i_usX2Y_subs_startup;
> }
> return 0;
> }
> @@ -485,12 +486,10 @@ static int usX2Y_urbs_start(struct snd_usX2Y_substream
> *subs)
> unsigned long pack;
> if (0 == i)
> atomic_set(>state, state_STARTING3);
> - urb->dev = usX2Y->dev;
> for (pack = 0; pack < nr_of_packs(); pack++) {
> urb->iso_frame_desc[pack].offset =
> subs->maxpacksize * pack;
> urb->iso_frame_desc[pack].length =
> subs->maxpacksize;
> }
> - urb->transfer_buffer_length = subs->maxpacksize *
> nr_of_packs();
> if ((err = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
> snd_printk (KERN_ERR "cannot submit datapipe
> for urb %d, err = %d\n", i, err);
> err = -EPIPE;
> --
> 2.17.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: UAC2 gadget not recognized on Windows 10
On Wed, Jun 20, 2018 at 9:16 PM, jonsm...@gmail.com wrote: > On Wed, Jun 20, 2018 at 11:19 AM Jassi Brar wrote: >> >> On Wed, Jun 20, 2018 at 8:32 PM, jonsm...@gmail.com >> wrote: >> > >> > >> > >> > On Wed, Jun 20, 2018 at 9:36 AM Jassi Brar >> > wrote: >> >> >> >> On Tue, Feb 13, 2018 at 3:02 PM, Robert Bielik >> >> wrote: >> >> >> Enabling SOF interrupts will be a big pain :-) Well, enabling the >> >> >> interrupt itself is a no-brainer, but it'll cause terrible CPU >> >> >> overload. >> >> > >> >> > Oh, I see. Hmm... would it be possible to allow upper levels to config >> >> > this dynamically ? I.e. for the ALSA subsystem there is no need for the >> >> > SOF timestamps, whereas for my proposal they would be needed. >> >> > >> >> > And what kind of CPU overhead are we talking about ? The IRQs >> >> > shouldn't come more often than every 125 us, and all that is needed is >> >> > to take a timestamp value But I'm probably overlooking a lot of stuff... >> >> > >> >> I believe we could control data rate precisely enough for the >> >> feeedback ep to be needed only for compatibility with Windows (Linux >> >> is already tested to work fine, MacOS is reported to have worked when >> >> the code was upstreamed.). >> > >> > >> > Right now Windows 10 (all I have checked) refuses to even connect to a >> > Linux based UAC2 device. It appears to be complaining that no endpoint >> > supporting USB_ENDPOINT_USAGE_FEEDBACK mode is implement. My five minutes >> > of poking in the spec implies that implementing this is required for an >> > endpoint doing USB_ENDPOINT_SYNC_ASYNC. Maybe this is something that can >> > be faked to the point of allowing Windows to connect to the UAC2 gadget >> > driver? >> > >> Thats what I meant. >> >> > The Windows error you get is: >> > "The driver could not find a feedback endpoint for an asynchronous data >> > OUT endpoint on device \Device\." >> > >> For OUT, there might be some hoops to jump. Honestly it's been years >> since I read the spec and got the code to work. I find it tempting to >> look into Windows 10 as well, but I can't commit to a timeline. > > For my application there is no recording HW on the gadget. Maybe a > quick way to fix this would be to make a flag to say that the gadget > is output only? And then change the descriptors around? > There could be hacks to fool Windows but the right approach is to find some real uac2 "USB-IN only" device and share the lsusb output. Then we could kill some descriptor to pretend like that device. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On Wed, Jun 20, 2018 at 02:54:10PM +0200, Rafael J. Wysocki wrote: > On Wednesday, June 20, 2018 2:23:46 PM CEST Johan Hovold wrote: > > On Wed, Jun 20, 2018 at 02:16:59AM -0700, Tony Lindgren wrote: > > > Hi, > > > > > > Adding Rafael and linux-pm to Cc as well. > > > > > > * Felipe Balbi [180619 01:23]: > > > > This is a direct consequence of not paying attention to the order of > > > > things. If driver were to assume that pm_domain->activate() would do the > > > > right thing for the device -- meaning that probe would run with an > > > > active device --, then we wouldn't need that pm_runtime_get() call on > > > > probe at all. Rather we would follow the sequence: > > > > > > > > pm_runtime_forbid() > > > > pm_runtime_set_active() > > > > pm_runtime_enable() > > > > > > > > /* do your probe routine */ > > > > > > > > pm_runtime_put_noidle() > > > > > > > > Then you remove you would need to call pm_runtime_get_noresume() to > > > > balance out the pm_runtime_put_noidle() there. > > > > > > (If you need to know why the pm_runtime_put_noidle(), remember that > > > > pm_runtime_set_active() increments the usage counter, so > > > > pm_runtime_put_noidle is basically allowing pm_runtime to happen as soon > > > > as userspace writes "auto" to /sys//power/control) > > > > That's not correct; pm_runtime_set_active() only increments the usage > > counter of a parent (under some circumstances), so unless you have bus > > code incrementing the usage counter before probe, the above > > pm_runtime_put_noidle() would actually introduce an imbalance. > > No, it wouldn't. It balances the incrementation in pm_runtime_forbid(). Right, but even if you take the whole sequence, which included pm_runtime_forbid(), consider what happens when pm_runtime_allow() is later called through sysfs (see below). > > And note that that's also the case even if you meant to say that > > *pm_runtime_forbid()* increments the usage counter (which it does). > > Why is it? > > Surely, after > > pm_runtime_forbid(dev); > pm_runtime_put_noidle(dev); > > the runtime PM usage counter of dev will be the same as before, won't it? Sure, but the imbalance, or rather inconsistent state, has already been introduced. Consider the following sequence of events: usage count 0 probe() pm_runtime_forbid() 1 pm_runtime_set_active() pm_runtime_enable() pm_runtime_put_noidle() 0 Here nothing is preventing the device from runtime suspending, despite runtime PM being forbidden. In fact, it will typically be suspended due to the pm_request_idle() in driver_probe_device(). If later we have: echo auto > power/control pm_runtime_allow() -1 then the device remains suspended, but we get a negative usage count. This does not seem to play well with neither runtime pm (consider a synchronous get followed by a put, which will fail to again suspend the device) or, for example, system suspend, which tries to prevent runtime pm by incrementing the usage count. And if runtime pm is later again forbidden: echo on > power/control pm_runtime_forbid() 0 then the device will not be resumed. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/9] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()
On 2018-06-20 17:38:44 [+0200], To Takashi Iwai wrote:
> okay, as you wish:
I'm sorry, I compiled one patch and send the other. Here is the fixed
one.
> > thanks,
> >
> > Takashi
>
--- >8
Subject: [PATCH 8/9 v4] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()
Using usb_fill_int_urb() helps to find code which initializes an
URB. A grep for members of the struct (like ->complete) reveal lots
of other things, too.
The "&& !(*purb)->transfer_buffer" check has been removed because the
URB has been freshly allocated a few lines above so ->transfer_buffer
has to be NULL here.
The `dev' and `transfer_size' assignments have been moved from
usX2Y_urbs_start() to usX2Y_urbs_allocate() because they don't change
overtime.
The multiplication via check_mul_overflow() has been extracted from
kmalloc_array() to avoid two multiplication (one with overflow check and
one without for the length argument). This requires to change the type
`maxpacksize' to int so there is only one type involved in the
multiplication.
Cc: Jaroslav Kysela
Cc: Takashi Iwai
Signed-off-by: Sebastian Andrzej Siewior
---
sound/usb/usx2y/usbusx2yaudio.c | 23 +++
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
index 2b833054e3b0..de3a21444db3 100644
--- a/sound/usb/usx2y/usbusx2yaudio.c
+++ b/sound/usb/usx2y/usbusx2yaudio.c
@@ -425,6 +425,9 @@ static int usX2Y_urbs_allocate(struct snd_usX2Y_substream
*subs)
/* allocate and initialize data urbs */
for (i = 0; i < NRURBS; i++) {
struct urb **purb = subs->urb + i;
+ void *buf = NULL;
+ int len = 0;
+
if (*purb) {
usb_kill_urb(*purb);
continue;
@@ -434,22 +437,20 @@ static int usX2Y_urbs_allocate(struct snd_usX2Y_substream
*subs)
usX2Y_urbs_release(subs);
return -ENOMEM;
}
- if (!is_playback && !(*purb)->transfer_buffer) {
+
+ if (!is_playback) {
/* allocate a capture buffer per urb */
- (*purb)->transfer_buffer =
- kmalloc_array(subs->maxpacksize,
- nr_of_packs(), GFP_KERNEL);
- if (NULL == (*purb)->transfer_buffer) {
+ buf = kmalloc_array(subs->maxpacksize, nr_of_packs(),
+ GFP_KERNEL);
+ if (!buf) {
usX2Y_urbs_release(subs);
return -ENOMEM;
}
+ len = subs->maxpacksize * nr_of_packs();
}
- (*purb)->dev = dev;
- (*purb)->pipe = pipe;
+ usb_fill_int_urb(*purb, dev, pipe, buf, len,
+i_usX2Y_subs_startup, subs, 1);
(*purb)->number_of_packets = nr_of_packs();
- (*purb)->context = subs;
- (*purb)->interval = 1;
- (*purb)->complete = i_usX2Y_subs_startup;
}
return 0;
}
@@ -485,12 +486,10 @@ static int usX2Y_urbs_start(struct snd_usX2Y_substream
*subs)
unsigned long pack;
if (0 == i)
atomic_set(>state, state_STARTING3);
- urb->dev = usX2Y->dev;
for (pack = 0; pack < nr_of_packs(); pack++) {
urb->iso_frame_desc[pack].offset =
subs->maxpacksize * pack;
urb->iso_frame_desc[pack].length =
subs->maxpacksize;
}
- urb->transfer_buffer_length = subs->maxpacksize *
nr_of_packs();
if ((err = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
snd_printk (KERN_ERR "cannot submit datapipe
for urb %d, err = %d\n", i, err);
err = -EPIPE;
--
2.17.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: UAC2 gadget not recognized on Windows 10
On Wed, Jun 20, 2018 at 11:19 AM Jassi Brar wrote: > > On Wed, Jun 20, 2018 at 8:32 PM, jonsm...@gmail.com > wrote: > > > > > > > > On Wed, Jun 20, 2018 at 9:36 AM Jassi Brar wrote: > >> > >> On Tue, Feb 13, 2018 at 3:02 PM, Robert Bielik > >> wrote: > >> >> Enabling SOF interrupts will be a big pain :-) Well, enabling the > >> >> interrupt itself is a no-brainer, but it'll cause terrible CPU overload. > >> > > >> > Oh, I see. Hmm... would it be possible to allow upper levels to config > >> > this dynamically ? I.e. for the ALSA subsystem there is no need for the > >> > SOF timestamps, whereas for my proposal they would be needed. > >> > > >> > And what kind of CPU overhead are we talking about ? The IRQs shouldn't > >> > come more often than every 125 us, and all that is needed is to take a > >> > timestamp value But I'm probably overlooking a lot of stuff... > >> > > >> I believe we could control data rate precisely enough for the > >> feeedback ep to be needed only for compatibility with Windows (Linux > >> is already tested to work fine, MacOS is reported to have worked when > >> the code was upstreamed.). > > > > > > Right now Windows 10 (all I have checked) refuses to even connect to a > > Linux based UAC2 device. It appears to be complaining that no endpoint > > supporting USB_ENDPOINT_USAGE_FEEDBACK mode is implement. My five minutes > > of poking in the spec implies that implementing this is required for an > > endpoint doing USB_ENDPOINT_SYNC_ASYNC. Maybe this is something that can > > be faked to the point of allowing Windows to connect to the UAC2 gadget > > driver? > > > Thats what I meant. > > > The Windows error you get is: > > "The driver could not find a feedback endpoint for an asynchronous data OUT > > endpoint on device \Device\." > > > For OUT, there might be some hoops to jump. Honestly it's been years > since I read the spec and got the code to work. I find it tempting to > look into Windows 10 as well, but I can't commit to a timeline. For my application there is no recording HW on the gadget. Maybe a quick way to fix this would be to make a flag to say that the gadget is output only? And then change the descriptors around? That would also help with my specific hardware case where the USB controller only supports ISOCHRONOUS in a single direction. Limitation of an Allwinner chip. > > Cheers! -- Jon Smirl jonsm...@gmail.com -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: UAC2 gadget not recognized on Windows 10
On Wed, Jun 20, 2018 at 8:32 PM, jonsm...@gmail.com wrote: > > > > On Wed, Jun 20, 2018 at 9:36 AM Jassi Brar wrote: >> >> On Tue, Feb 13, 2018 at 3:02 PM, Robert Bielik >> wrote: >> >> Enabling SOF interrupts will be a big pain :-) Well, enabling the >> >> interrupt itself is a no-brainer, but it'll cause terrible CPU overload. >> > >> > Oh, I see. Hmm... would it be possible to allow upper levels to config >> > this dynamically ? I.e. for the ALSA subsystem there is no need for the >> > SOF timestamps, whereas for my proposal they would be needed. >> > >> > And what kind of CPU overhead are we talking about ? The IRQs shouldn't >> > come more often than every 125 us, and all that is needed is to take a >> > timestamp value But I'm probably overlooking a lot of stuff... >> > >> I believe we could control data rate precisely enough for the >> feeedback ep to be needed only for compatibility with Windows (Linux >> is already tested to work fine, MacOS is reported to have worked when >> the code was upstreamed.). > > > Right now Windows 10 (all I have checked) refuses to even connect to a Linux > based UAC2 device. It appears to be complaining that no endpoint supporting > USB_ENDPOINT_USAGE_FEEDBACK mode is implement. My five minutes of poking in > the spec implies that implementing this is required for an endpoint doing > USB_ENDPOINT_SYNC_ASYNC. Maybe this is something that can be faked to the > point of allowing Windows to connect to the UAC2 gadget driver? > Thats what I meant. > The Windows error you get is: > "The driver could not find a feedback endpoint for an asynchronous data OUT > endpoint on device \Device\." > For OUT, there might be some hoops to jump. Honestly it's been years since I read the spec and got the code to work. I find it tempting to look into Windows 10 as well, but I can't commit to a timeline. Cheers! -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/9] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()
On Wed, 20 Jun 2018 16:39:22 +0200,
Sebastian Andrzej Siewior wrote:
>
> On 2018-06-20 14:51:12 [+0200], Takashi Iwai wrote:
> > On Tue, 19 Jun 2018 23:55:20 +0200,
> > Sebastian Andrzej Siewior wrote:
> > > - (*purb)->transfer_buffer =
> > > - kmalloc_array(subs->maxpacksize,
> > > - nr_of_packs(), GFP_KERNEL);
> > > - if (NULL == (*purb)->transfer_buffer) {
> > > + len = subs->maxpacksize * nr_of_packs();
> > > + buf = kmalloc(len, GFP_KERNEL);
> >
> > I'd keep kmalloc_array() as is, and just put subs->maxpacksize *
> > nr_of_packs() in usb_fill_int_urb(). Otherwise it's a step backward.
>
> but then we end up with two multiplications.
Yes, but it's no hot path, and the code won't bigger.
And I bet you won't notice any performance lost by that :)
> What about pulling the
> overflow-mul macro out of malloc_array() and doing this instead:
Well, it's neither smaller nor more readable...
thanks,
Takashi
>
> --- >8
>
> Subject: [PATCH 8/9 v2] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()
>
> Using usb_fill_int_urb() helps to find code which initializes an
> URB. A grep for members of the struct (like ->complete) reveal lots
> of other things, too.
>
> The "&& !(*purb)->transfer_buffer" check has been removed because the
> URB has been freshly allocated a few lines above so ->transfer_buffer
> has to be NULL here.
> The `dev' and `transfer_size' assignments have been moved from
> usX2Y_urbs_start() to usX2Y_urbs_allocate() because they don't change
> overtime.
> The multiplication via check_mul_overflow() has been extracted from
> kmalloc_array() to avoid two multiplication (one with overflow check and
> one without for the length argument). This requires to change the type
> `maxpacksize' to int so there is only one type involved in the
> multiplication.
>
> Cc: Jaroslav Kysela
> Cc: Takashi Iwai
> Signed-off-by: Sebastian Andrzej Siewior
> ---
> sound/usb/usx2y/usbusx2y.h | 2 +-
> sound/usb/usx2y/usbusx2yaudio.c | 38 -
> 2 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/sound/usb/usx2y/usbusx2y.h b/sound/usb/usx2y/usbusx2y.h
> index e0f77172ce8f..2bec412589b4 100644
> --- a/sound/usb/usx2y/usbusx2y.h
> +++ b/sound/usb/usx2y/usbusx2y.h
> @@ -56,7 +56,7 @@ struct snd_usX2Y_substream {
> struct snd_pcm_substream *pcm_substream;
>
> int endpoint;
> - unsigned intmaxpacksize;/* max packet size in
> bytes */
> + int maxpacksize;/* max packet size in
> bytes */
>
> atomic_tstate;
> #define state_STOPPED0
> diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
> index 2b833054e3b0..e5580cb59858 100644
> --- a/sound/usb/usx2y/usbusx2yaudio.c
> +++ b/sound/usb/usx2y/usbusx2yaudio.c
> @@ -425,33 +425,35 @@ static int usX2Y_urbs_allocate(struct
> snd_usX2Y_substream *subs)
> /* allocate and initialize data urbs */
> for (i = 0; i < NRURBS; i++) {
> struct urb **purb = subs->urb + i;
> + void *buf = NULL;
> + int len = 0;
> +
> if (*purb) {
> usb_kill_urb(*purb);
> continue;
> }
> *purb = usb_alloc_urb(nr_of_packs(), GFP_KERNEL);
> - if (NULL == *purb) {
> - usX2Y_urbs_release(subs);
> - return -ENOMEM;
> - }
> - if (!is_playback && !(*purb)->transfer_buffer) {
> + if (NULL == *purb)
> + goto err_free;
> +
> + if (!is_playback) {
> /* allocate a capture buffer per urb */
> - (*purb)->transfer_buffer =
> - kmalloc_array(subs->maxpacksize,
> - nr_of_packs(), GFP_KERNEL);
> - if (NULL == (*purb)->transfer_buffer) {
> - usX2Y_urbs_release(subs);
> - return -ENOMEM;
> - }
> + if (check_mul_overflow(subs->maxpacksize,
> +nr_of_packs(),
> +))
> + goto err_free;
> + buf = kmalloc(len, GFP_KERNEL);
> + if (!buf)
> + goto err_free;
> }
> - (*purb)->dev = dev;
> - (*purb)->pipe = pipe;
> + usb_fill_int_urb(*purb, dev, pipe, buf, len,
> + i_usX2Y_subs_startup, subs, 1);
> (*purb)->number_of_packets = nr_of_packs();
> - (*purb)->context = subs;
> - (*purb)->interval = 1;
> -
Re: [PATCH 8/9] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()
On 2018-06-20 14:51:12 [+0200], Takashi Iwai wrote:
> On Tue, 19 Jun 2018 23:55:20 +0200,
> Sebastian Andrzej Siewior wrote:
> > - (*purb)->transfer_buffer =
> > - kmalloc_array(subs->maxpacksize,
> > - nr_of_packs(), GFP_KERNEL);
> > - if (NULL == (*purb)->transfer_buffer) {
> > + len = subs->maxpacksize * nr_of_packs();
> > + buf = kmalloc(len, GFP_KERNEL);
>
> I'd keep kmalloc_array() as is, and just put subs->maxpacksize *
> nr_of_packs() in usb_fill_int_urb(). Otherwise it's a step backward.
but then we end up with two multiplications. What about pulling the
overflow-mul macro out of malloc_array() and doing this instead:
--- >8
Subject: [PATCH 8/9 v2] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()
Using usb_fill_int_urb() helps to find code which initializes an
URB. A grep for members of the struct (like ->complete) reveal lots
of other things, too.
The "&& !(*purb)->transfer_buffer" check has been removed because the
URB has been freshly allocated a few lines above so ->transfer_buffer
has to be NULL here.
The `dev' and `transfer_size' assignments have been moved from
usX2Y_urbs_start() to usX2Y_urbs_allocate() because they don't change
overtime.
The multiplication via check_mul_overflow() has been extracted from
kmalloc_array() to avoid two multiplication (one with overflow check and
one without for the length argument). This requires to change the type
`maxpacksize' to int so there is only one type involved in the
multiplication.
Cc: Jaroslav Kysela
Cc: Takashi Iwai
Signed-off-by: Sebastian Andrzej Siewior
---
sound/usb/usx2y/usbusx2y.h | 2 +-
sound/usb/usx2y/usbusx2yaudio.c | 38 -
2 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/sound/usb/usx2y/usbusx2y.h b/sound/usb/usx2y/usbusx2y.h
index e0f77172ce8f..2bec412589b4 100644
--- a/sound/usb/usx2y/usbusx2y.h
+++ b/sound/usb/usx2y/usbusx2y.h
@@ -56,7 +56,7 @@ struct snd_usX2Y_substream {
struct snd_pcm_substream *pcm_substream;
int endpoint;
- unsigned intmaxpacksize;/* max packet size in
bytes */
+ int maxpacksize;/* max packet size in
bytes */
atomic_tstate;
#define state_STOPPED 0
diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
index 2b833054e3b0..e5580cb59858 100644
--- a/sound/usb/usx2y/usbusx2yaudio.c
+++ b/sound/usb/usx2y/usbusx2yaudio.c
@@ -425,33 +425,35 @@ static int usX2Y_urbs_allocate(struct snd_usX2Y_substream
*subs)
/* allocate and initialize data urbs */
for (i = 0; i < NRURBS; i++) {
struct urb **purb = subs->urb + i;
+ void *buf = NULL;
+ int len = 0;
+
if (*purb) {
usb_kill_urb(*purb);
continue;
}
*purb = usb_alloc_urb(nr_of_packs(), GFP_KERNEL);
- if (NULL == *purb) {
- usX2Y_urbs_release(subs);
- return -ENOMEM;
- }
- if (!is_playback && !(*purb)->transfer_buffer) {
+ if (NULL == *purb)
+ goto err_free;
+
+ if (!is_playback) {
/* allocate a capture buffer per urb */
- (*purb)->transfer_buffer =
- kmalloc_array(subs->maxpacksize,
- nr_of_packs(), GFP_KERNEL);
- if (NULL == (*purb)->transfer_buffer) {
- usX2Y_urbs_release(subs);
- return -ENOMEM;
- }
+ if (check_mul_overflow(subs->maxpacksize,
+ nr_of_packs(),
+ ))
+ goto err_free;
+ buf = kmalloc(len, GFP_KERNEL);
+ if (!buf)
+ goto err_free;
}
- (*purb)->dev = dev;
- (*purb)->pipe = pipe;
+ usb_fill_int_urb(*purb, dev, pipe, buf, len,
+i_usX2Y_subs_startup, subs, 1);
(*purb)->number_of_packets = nr_of_packs();
- (*purb)->context = subs;
- (*purb)->interval = 1;
- (*purb)->complete = i_usX2Y_subs_startup;
}
return 0;
+err_free:
+ usX2Y_urbs_release(subs);
+ return -ENOMEM;
}
static void usX2Y_subs_startup(struct snd_usX2Y_substream *subs)
@@ -485,12 +487,10 @@ static int usX2Y_urbs_start(struct snd_usX2Y_substream
*subs)
unsigned long pack;
if
Re: [PATCH 4/9 v2] ALSA: usb-audio: use usb_fill_int_urb()
On Wed, 20 Jun 2018 15:56:53 +0200, Takashi Iwai wrote: > > On Wed, 20 Jun 2018 15:52:49 +0200, > Sebastian Andrzej Siewior wrote: > > > > On 2018-06-20 15:21:49 [+0200], Takashi Iwai wrote: > > > > usb_fill_int_urb() ensures that syncinterval is within the allowed range > > > > on HS/SS. The interval value seems to come from > > > > snd_usb_parse_datainterval() which is bInterval - 1 and only in the rage > > > > 1 … 4. So in order to keep the magic working I pass datainterval + 1. > > > > > > This needs more explanation. By this conversion, the interval > > > computation becomes really tricky. > > > > > > There are two interval calculations. The first one is > > > fp->datainterval and it's from snd_usb_parse_datainterval() as you > > > mentioned. And a tricky part is that fp->datainterval is 0 on > > > FULL_SPEED. Casually, fp->datainterval+1 will be translated to the > > > correct value since (0 + 1) == (1 << 0). > > > > > > OTOH, for the sync EP, it's taken from ep->syncinterval, which is set > > > in snd_usb_add_endpoint(). Luckily, again, ep->syncinterval + 1 works > > > for FULL_SPEED as well, as (1 + 1) == (1 << 1). > > > > Do you want me to add your additional explanation to the description? > > Yes. It's a very implicit assumption, and needs clarification. > In addition, the comment 1...4 is only for fp->datainterval, not about > ep->syncinterval. > > Alternatively, give some comments in the places where putting > fp->datainterval and ep->syncinterval. Oh, and for other patches, something about interval could be mentioned in the change log. You pass 1 to the macro as if it were identical as the original code (urb->interval = 1), but this isn't evaluated equally. Effectively it'll result in the same, but better to be clarified. thanks, Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/9 v2] ALSA: usb-audio: use usb_fill_int_urb()
On Wed, 20 Jun 2018 15:52:49 +0200, Sebastian Andrzej Siewior wrote: > > On 2018-06-20 15:21:49 [+0200], Takashi Iwai wrote: > > > usb_fill_int_urb() ensures that syncinterval is within the allowed range > > > on HS/SS. The interval value seems to come from > > > snd_usb_parse_datainterval() which is bInterval - 1 and only in the rage > > > 1 … 4. So in order to keep the magic working I pass datainterval + 1. > > > > This needs more explanation. By this conversion, the interval > > computation becomes really tricky. > > > > There are two interval calculations. The first one is > > fp->datainterval and it's from snd_usb_parse_datainterval() as you > > mentioned. And a tricky part is that fp->datainterval is 0 on > > FULL_SPEED. Casually, fp->datainterval+1 will be translated to the > > correct value since (0 + 1) == (1 << 0). > > > > OTOH, for the sync EP, it's taken from ep->syncinterval, which is set > > in snd_usb_add_endpoint(). Luckily, again, ep->syncinterval + 1 works > > for FULL_SPEED as well, as (1 + 1) == (1 << 1). > > Do you want me to add your additional explanation to the description? Yes. It's a very implicit assumption, and needs clarification. In addition, the comment 1...4 is only for fp->datainterval, not about ep->syncinterval. Alternatively, give some comments in the places where putting fp->datainterval and ep->syncinterval. thanks, Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/9 v2] ALSA: usb-audio: use usb_fill_int_urb()
On 2018-06-20 15:21:49 [+0200], Takashi Iwai wrote: > > usb_fill_int_urb() ensures that syncinterval is within the allowed range > > on HS/SS. The interval value seems to come from > > snd_usb_parse_datainterval() which is bInterval - 1 and only in the rage > > 1 … 4. So in order to keep the magic working I pass datainterval + 1. > > This needs more explanation. By this conversion, the interval > computation becomes really tricky. > > There are two interval calculations. The first one is > fp->datainterval and it's from snd_usb_parse_datainterval() as you > mentioned. And a tricky part is that fp->datainterval is 0 on > FULL_SPEED. Casually, fp->datainterval+1 will be translated to the > correct value since (0 + 1) == (1 << 0). > > OTOH, for the sync EP, it's taken from ep->syncinterval, which is set > in snd_usb_add_endpoint(). Luckily, again, ep->syncinterval + 1 works > for FULL_SPEED as well, as (1 + 1) == (1 << 1). Do you want me to add your additional explanation to the description? > thanks, > > Takashi Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: UAC2 gadget not recognized on Windows 10
On Tue, Feb 13, 2018 at 3:02 PM, Robert Bielik wrote: >> Enabling SOF interrupts will be a big pain :-) Well, enabling the >> interrupt itself is a no-brainer, but it'll cause terrible CPU overload. > > Oh, I see. Hmm... would it be possible to allow upper levels to config this > dynamically ? I.e. for the ALSA subsystem there is no need for the SOF > timestamps, whereas for my proposal they would be needed. > > And what kind of CPU overhead are we talking about ? The IRQs shouldn't come > more often than every 125 us, and all that is needed is to take a timestamp > value But I'm probably overlooking a lot of stuff... > I believe we could control data rate precisely enough for the feeedback ep to be needed only for compatibility with Windows (Linux is already tested to work fine, MacOS is reported to have worked when the code was upstreamed.). There was an implementation suggested by Alan Stern which would maintain accurate rate over a reasonable period of time. But that was rejected ... https://patches.linaro.org/patch/36270/ -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/9 v2] ALSA: usb-audio: use usb_fill_int_urb()
On Wed, 20 Jun 2018 11:38:27 +0200,
Sebastian Andrzej Siewior wrote:
>
> Using usb_fill_int_urb() helps to find code which initializes an
> URB. A grep for members of the struct (like ->complete) reveal lots
> of other things, too.
>
> data_ep_set_params() additionally sets urb->transfer_buffer_length which
> was not the case earlier.
> usb_fill_int_urb() ensures that syncinterval is within the allowed range
> on HS/SS. The interval value seems to come from
> snd_usb_parse_datainterval() which is bInterval - 1 and only in the rage
> 1 … 4. So in order to keep the magic working I pass datainterval + 1.
This needs more explanation. By this conversion, the interval
computation becomes really tricky.
There are two interval calculations. The first one is
fp->datainterval and it's from snd_usb_parse_datainterval() as you
mentioned. And a tricky part is that fp->datainterval is 0 on
FULL_SPEED. Casually, fp->datainterval+1 will be translated to the
correct value since (0 + 1) == (1 << 0).
OTOH, for the sync EP, it's taken from ep->syncinterval, which is set
in snd_usb_add_endpoint(). Luckily, again, ep->syncinterval + 1 works
for FULL_SPEED as well, as (1 + 1) == (1 << 1).
thanks,
Takashi
> Cc: Jaroslav Kysela
> Cc: Takashi Iwai
> Signed-off-by: Sebastian Andrzej Siewior
> ---
> v1…v2: description changes (spelling + usb_fill_int_urb() ensures
>"syncinterval" not data_ep_set_params())
>
> sound/usb/endpoint.c | 24 ++--
> 1 file changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
> index c90607ebe155..bbc02db5b417 100644
> --- a/sound/usb/endpoint.c
> +++ b/sound/usb/endpoint.c
> @@ -772,6 +772,8 @@ static int data_ep_set_params(struct snd_usb_endpoint *ep,
> /* allocate and initialize data urbs */
> for (i = 0; i < ep->nurbs; i++) {
> struct snd_urb_ctx *u = >urb[i];
> + void *buf;
> +
> u->index = i;
> u->ep = ep;
> u->packets = urb_packs;
> @@ -783,16 +785,13 @@ static int data_ep_set_params(struct snd_usb_endpoint
> *ep,
> if (!u->urb)
> goto out_of_memory;
>
> - u->urb->transfer_buffer =
> - usb_alloc_coherent(ep->chip->dev, u->buffer_size,
> -GFP_KERNEL, >urb->transfer_dma);
> - if (!u->urb->transfer_buffer)
> + buf = usb_alloc_coherent(ep->chip->dev, u->buffer_size,
> + GFP_KERNEL, >urb->transfer_dma);
> + if (!buf)
> goto out_of_memory;
> - u->urb->pipe = ep->pipe;
> + usb_fill_int_urb(u->urb, NULL, ep->pipe, buf, u->buffer_size,
> + snd_complete_urb, u, ep->datainterval + 1);
> u->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
> - u->urb->interval = 1 << ep->datainterval;
> - u->urb->context = u;
> - u->urb->complete = snd_complete_urb;
> INIT_LIST_HEAD(>ready_list);
> }
>
> @@ -823,15 +822,12 @@ static int sync_ep_set_params(struct snd_usb_endpoint
> *ep)
> u->urb = usb_alloc_urb(1, GFP_KERNEL);
> if (!u->urb)
> goto out_of_memory;
> - u->urb->transfer_buffer = ep->syncbuf + i * 4;
> + usb_fill_int_urb(u->urb, NULL, ep->pipe, ep->syncbuf + i * 4, 4,
> + snd_complete_urb, u, ep->syncinterval + 1);
> +
> u->urb->transfer_dma = ep->sync_dma + i * 4;
> - u->urb->transfer_buffer_length = 4;
> - u->urb->pipe = ep->pipe;
> u->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
> u->urb->number_of_packets = 1;
> - u->urb->interval = 1 << ep->syncinterval;
> - u->urb->context = u;
> - u->urb->complete = snd_complete_urb;
> }
>
> ep->nurbs = SYNC_URBS;
> --
> 2.17.1
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ALSA: use irqsave() in URB completion + usb_fill_int_urb
On Tue, 19 Jun 2018 23:55:12 +0200, Sebastian Andrzej Siewior wrote: > > This series is mostly about using _irqsave() primitives in the > completion callback in order to get rid of local_irq_save() in > __usb_hcd_giveback_urb(). While at it, I also tried to move drivers to > use usb_fill_int_urb() otherwise it is hard find users of a certain API. At the next time, please split to two different patchsets in such a case. The conversions to usb_fill_int_urb() and the change to _irqsave() are completely different things; the former is merely a code cleanup that doesn't give any functional change, while the latter is the followup for the USB core change. thanks, Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On Wednesday, June 20, 2018 2:23:46 PM CEST Johan Hovold wrote: > On Wed, Jun 20, 2018 at 02:16:59AM -0700, Tony Lindgren wrote: > > Hi, > > > > Adding Rafael and linux-pm to Cc as well. > > > > * Felipe Balbi [180619 01:23]: > > > This is a direct consequence of not paying attention to the order of > > > things. If driver were to assume that pm_domain->activate() would do the > > > right thing for the device -- meaning that probe would run with an > > > active device --, then we wouldn't need that pm_runtime_get() call on > > > probe at all. Rather we would follow the sequence: > > > > > > pm_runtime_forbid() > > > pm_runtime_set_active() > > > pm_runtime_enable() > > > > > > /* do your probe routine */ > > > > > > pm_runtime_put_noidle() > > > > > > Then you remove you would need to call pm_runtime_get_noresume() to > > > balance out the pm_runtime_put_noidle() there. > > > > (If you need to know why the pm_runtime_put_noidle(), remember that > > > pm_runtime_set_active() increments the usage counter, so > > > pm_runtime_put_noidle is basically allowing pm_runtime to happen as soon > > > as userspace writes "auto" to /sys//power/control) > > That's not correct; pm_runtime_set_active() only increments the usage > counter of a parent (under some circumstances), so unless you have bus > code incrementing the usage counter before probe, the above > pm_runtime_put_noidle() would actually introduce an imbalance. No, it wouldn't. It balances the incrementation in pm_runtime_forbid(). > And note that that's also the case even if you meant to say that > *pm_runtime_forbid()* increments the usage counter (which it does). Why is it? Surely, after pm_runtime_forbid(dev); pm_runtime_put_noidle(dev); the runtime PM usage counter of dev will be the same as before, won't it? -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/9] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()
On Tue, 19 Jun 2018 23:55:20 +0200,
Sebastian Andrzej Siewior wrote:
>
> Using usb_fill_int_urb() helps to find code which initializes an
> URB. A grep for members of the struct (like ->complete) reveal lots
> of other things, too.
>
> The "&& !(*purb)->transfer_buffer" check has been removed because the
> URB has been freshly allocated a few lines above so ->transfer_buffer
> has to be NULL here.
> The `dev' and `transfer_size' assignments have been moved from
> usX2Y_urbs_start() to usX2Y_urbs_allocate() because they don't change
> overtime.
>
> Cc: Jaroslav Kysela
> Cc: Takashi Iwai
> Signed-off-by: Sebastian Andrzej Siewior
> ---
> sound/usb/usx2y/usbusx2yaudio.c | 21 +
> 1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
> index 2b833054e3b0..9a49bdb07508 100644
> --- a/sound/usb/usx2y/usbusx2yaudio.c
> +++ b/sound/usb/usx2y/usbusx2yaudio.c
> @@ -425,6 +425,9 @@ static int usX2Y_urbs_allocate(struct snd_usX2Y_substream
> *subs)
> /* allocate and initialize data urbs */
> for (i = 0; i < NRURBS; i++) {
> struct urb **purb = subs->urb + i;
> + void *buf = NULL;
> + unsigned int len = 0;
> +
> if (*purb) {
> usb_kill_urb(*purb);
> continue;
> @@ -434,22 +437,18 @@ static int usX2Y_urbs_allocate(struct
> snd_usX2Y_substream *subs)
> usX2Y_urbs_release(subs);
> return -ENOMEM;
> }
> - if (!is_playback && !(*purb)->transfer_buffer) {
> + if (!is_playback) {
> /* allocate a capture buffer per urb */
> - (*purb)->transfer_buffer =
> - kmalloc_array(subs->maxpacksize,
> - nr_of_packs(), GFP_KERNEL);
> - if (NULL == (*purb)->transfer_buffer) {
> + len = subs->maxpacksize * nr_of_packs();
> + buf = kmalloc(len, GFP_KERNEL);
I'd keep kmalloc_array() as is, and just put subs->maxpacksize *
nr_of_packs() in usb_fill_int_urb(). Otherwise it's a step backward.
thanks,
Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On Wed, Jun 20, 2018 at 02:16:59AM -0700, Tony Lindgren wrote: > Hi, > > Adding Rafael and linux-pm to Cc as well. > > * Felipe Balbi [180619 01:23]: > > This is a direct consequence of not paying attention to the order of > > things. If driver were to assume that pm_domain->activate() would do the > > right thing for the device -- meaning that probe would run with an > > active device --, then we wouldn't need that pm_runtime_get() call on > > probe at all. Rather we would follow the sequence: > > > > pm_runtime_forbid() > > pm_runtime_set_active() > > pm_runtime_enable() > > > > /* do your probe routine */ > > > > pm_runtime_put_noidle() > > > > Then you remove you would need to call pm_runtime_get_noresume() to > > balance out the pm_runtime_put_noidle() there. > > (If you need to know why the pm_runtime_put_noidle(), remember that > > pm_runtime_set_active() increments the usage counter, so > > pm_runtime_put_noidle is basically allowing pm_runtime to happen as soon > > as userspace writes "auto" to /sys//power/control) That's not correct; pm_runtime_set_active() only increments the usage counter of a parent (under some circumstances), so unless you have bus code incrementing the usage counter before probe, the above pm_runtime_put_noidle() would actually introduce an imbalance. And note that that's also the case even if you meant to say that *pm_runtime_forbid()* increments the usage counter (which it does). Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On Wed, Jun 20, 2018 at 1:05 PM, Felipe Balbi wrote: > "Rafael J. Wysocki" writes: > >> On Wed, Jun 20, 2018 at 11:27 AM, Felipe Balbi wrote: >>> >>> Hi, >>> >>> Tony Lindgren writes: * Felipe Balbi [180619 01:23]: > This is a direct consequence of not paying attention to the order of > things. If driver were to assume that pm_domain->activate() would do the > right thing for the device -- meaning that probe would run with an > active device --, then we wouldn't need that pm_runtime_get() call on > probe at all. Rather we would follow the sequence: > > pm_runtime_forbid() >> >> Why do you need the _forbid() thing? >> >> Does the default user space control setting need to be changed? > > wouldn't that race with a udev rule writing "auto" power/control as soon > as device is added? Why would it? > (If you need to know why the pm_runtime_put_noidle(), remember that > pm_runtime_set_active() increments the usage counter, so > pm_runtime_put_noidle is basically allowing pm_runtime to happen as soon > as userspace writes "auto" to /sys//power/control) I wonder if we could also remove the need for drivers to call pm_runtime_putnoidle() at the end of the probe? If we had pm_runtime_init_enabled() implemented. >>> >>> probably not. We want to block runtime pm during probe, until the device >>> is fully initialized, the only way to do that is to increment rpm usage >>> counter. >> >> That or enable it only at the end. But I guess you want to >> runtime-resume it earlier, don't you? > > well, if arch implements pm_domain->activate() and guarantees that > device is already running, with clocks stable during probe, then > enabling runtime pm at the end is probably the best idea. But if you call pm_runtime_set_active() at any point, you're assuming that the device is active anyway. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PULL] USB fixes for v4.18-rc1
Hi Greg, here's the first set of fixes for current -rc cycle. Patches have been soaking for a while and I've also tested on some of our platforms from the lab. Let me know if you want anything to be changed. cheers The following changes since commit ce397d215ccd07b8ae3f71db689aedb85d56ab40: Linux 4.18-rc1 (2018-06-17 08:04:49 +0900) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git fixes-for-v4.18-rc1 for you to fetch changes up to 1d8e5c00275825fc42aaa5597dab1d0b5b26bb64: dwc2: gadget: Fix ISOC IN DDMA PID bitfield value calculation (2018-06-19 12:48:14 +0300) usb: fixes for v4.18-rc1 First set of fixes for the current -rc cycle. The main parts being warnings of different kinds being fixed. We're also adding support for Intel'l Icelake devices on dwc3-pci.c. Arnd Bergmann (1): usb: dwc3: qcom: mark PM functions as __maybe_unused Artur Petrosyan (1): usb: dwc2: Fix host exit from hibernation flow. Chunfeng Yun (1): usb: gadget: composite: fix delayed_status race condition when set_interface Grigor Tovmasyan (1): usb: gadget: dwc2: fix memory leak in gadget_init() Hans de Goede (1): usb: dwc3: Only call clk_bulk_get() on devicetree instantiated devices Heikki Krogerus (1): usb: dwc3: pci: add support for Intel IceLake Jaejoong Kim (1): doc: usb: Fix typo in gadget_configfs documentation Johan Hovold (1): usb: dwc3: of-simple: fix use-after-free on remove Minas Harutyunyan (3): usb: dwc2: gadget: Fix issue in dwc2_gadget_start_isoc() usb: dwc2: gadget: fix packet drop issue for ISOC OUT transfers dwc2: gadget: Fix ISOC IN DDMA PID bitfield value calculation Vincent Pelletier (1): usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers Wei Yongjun (1): usb: dwc3: Fix error return code in dwc3_qcom_probe() William Wu (3): usb: dwc2: fix the incorrect bitmaps for the ports of multi_tt hub usb: dwc2: alloc dma aligned buffer for isoc split in usb: dwc2: fix isoc split in transfer with no data Zeng Tao (1): usb: dwc2: gadget: fix packet drop issue in dwc2_gadget_handle_nak Documentation/usb/gadget_configfs.txt | 2 +- drivers/usb/dwc2/core.h | 3 ++ drivers/usb/dwc2/gadget.c | 20 +--- drivers/usb/dwc2/hcd.c| 93 --- drivers/usb/dwc2/hcd.h| 8 +++ drivers/usb/dwc2/hcd_intr.c | 11 - drivers/usb/dwc2/hcd_queue.c | 5 +- drivers/usb/dwc3/core.c | 23 + drivers/usb/dwc3/dwc3-of-simple.c | 3 +- drivers/usb/dwc3/dwc3-pci.c | 2 + drivers/usb/dwc3/dwc3-qcom.c | 13 ++--- drivers/usb/gadget/composite.c| 3 ++ drivers/usb/gadget/function/f_fs.c| 26 +++--- 13 files changed, 167 insertions(+), 45 deletions(-) -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
"Rafael J. Wysocki" writes: > On Wed, Jun 20, 2018 at 11:27 AM, Felipe Balbi wrote: >> >> Hi, >> >> Tony Lindgren writes: >>> * Felipe Balbi [180619 01:23]: This is a direct consequence of not paying attention to the order of things. If driver were to assume that pm_domain->activate() would do the right thing for the device -- meaning that probe would run with an active device --, then we wouldn't need that pm_runtime_get() call on probe at all. Rather we would follow the sequence: pm_runtime_forbid() > > Why do you need the _forbid() thing? > > Does the default user space control setting need to be changed? wouldn't that race with a udev rule writing "auto" power/control as soon as device is added? (If you need to know why the pm_runtime_put_noidle(), remember that pm_runtime_set_active() increments the usage counter, so pm_runtime_put_noidle is basically allowing pm_runtime to happen as soon as userspace writes "auto" to /sys//power/control) >>> >>> I wonder if we could also remove the need for drivers to call >>> pm_runtime_putnoidle() at the end of the probe? If we had >>> pm_runtime_init_enabled() implemented. >> >> probably not. We want to block runtime pm during probe, until the device >> is fully initialized, the only way to do that is to increment rpm usage >> counter. > > That or enable it only at the end. But I guess you want to > runtime-resume it earlier, don't you? well, if arch implements pm_domain->activate() and guarantees that device is already running, with clocks stable during probe, then enabling runtime pm at the end is probably the best idea. -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On Wed, Jun 20, 2018 at 11:27 AM, Felipe Balbi wrote: > > Hi, > > Tony Lindgren writes: >> * Felipe Balbi [180619 01:23]: >>> This is a direct consequence of not paying attention to the order of >>> things. If driver were to assume that pm_domain->activate() would do the >>> right thing for the device -- meaning that probe would run with an >>> active device --, then we wouldn't need that pm_runtime_get() call on >>> probe at all. Rather we would follow the sequence: >>> >>> pm_runtime_forbid() Why do you need the _forbid() thing? Does the default user space control setting need to be changed? >>> pm_runtime_set_active() >>> pm_runtime_enable() >>> >>> /* do your probe routine */ >>> >>> pm_runtime_put_noidle() >>> >>> Then you remove you would need to call pm_runtime_get_noresume() to >>> balance out the pm_runtime_put_noidle() there. >> >> How about let's create some prettier interface for the above runtime PM >> trickery? >> >> How about something like pm_runtime_init_enabled() for the above >> sequence? And then have a separate helper for every other use case? Come on. >> It might be then able to do the trick even if activate is not >> implemented.. >> >> Right now it has the feeling of "oh well we can't get runtime PM to >> work so let's bypass it with activate call and then trick runtime PM >> to start in enabled mode" :) > > no strong feelings about that either way. It's only 3 lines of code > anyway. I feel like that's a minor cosmetic change. > >>> (If you need to know why the pm_runtime_put_noidle(), remember that >>> pm_runtime_set_active() increments the usage counter, so >>> pm_runtime_put_noidle is basically allowing pm_runtime to happen as soon >>> as userspace writes "auto" to /sys//power/control) >> >> I wonder if we could also remove the need for drivers to call >> pm_runtime_putnoidle() at the end of the probe? If we had >> pm_runtime_init_enabled() implemented. > > probably not. We want to block runtime pm during probe, until the device > is fully initialized, the only way to do that is to increment rpm usage > counter. That or enable it only at the end. But I guess you want to runtime-resume it earlier, don't you? -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/9 v2] ALSA: usb-audio: use usb_fill_int_urb()
Using usb_fill_int_urb() helps to find code which initializes an
URB. A grep for members of the struct (like ->complete) reveal lots
of other things, too.
data_ep_set_params() additionally sets urb->transfer_buffer_length which
was not the case earlier.
usb_fill_int_urb() ensures that syncinterval is within the allowed range
on HS/SS. The interval value seems to come from
snd_usb_parse_datainterval() which is bInterval - 1 and only in the rage
1 … 4. So in order to keep the magic working I pass datainterval + 1.
Cc: Jaroslav Kysela
Cc: Takashi Iwai
Signed-off-by: Sebastian Andrzej Siewior
---
v1…v2: description changes (spelling + usb_fill_int_urb() ensures
"syncinterval" not data_ep_set_params())
sound/usb/endpoint.c | 24 ++--
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index c90607ebe155..bbc02db5b417 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -772,6 +772,8 @@ static int data_ep_set_params(struct snd_usb_endpoint *ep,
/* allocate and initialize data urbs */
for (i = 0; i < ep->nurbs; i++) {
struct snd_urb_ctx *u = >urb[i];
+ void *buf;
+
u->index = i;
u->ep = ep;
u->packets = urb_packs;
@@ -783,16 +785,13 @@ static int data_ep_set_params(struct snd_usb_endpoint *ep,
if (!u->urb)
goto out_of_memory;
- u->urb->transfer_buffer =
- usb_alloc_coherent(ep->chip->dev, u->buffer_size,
- GFP_KERNEL, >urb->transfer_dma);
- if (!u->urb->transfer_buffer)
+ buf = usb_alloc_coherent(ep->chip->dev, u->buffer_size,
+GFP_KERNEL, >urb->transfer_dma);
+ if (!buf)
goto out_of_memory;
- u->urb->pipe = ep->pipe;
+ usb_fill_int_urb(u->urb, NULL, ep->pipe, buf, u->buffer_size,
+snd_complete_urb, u, ep->datainterval + 1);
u->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
- u->urb->interval = 1 << ep->datainterval;
- u->urb->context = u;
- u->urb->complete = snd_complete_urb;
INIT_LIST_HEAD(>ready_list);
}
@@ -823,15 +822,12 @@ static int sync_ep_set_params(struct snd_usb_endpoint *ep)
u->urb = usb_alloc_urb(1, GFP_KERNEL);
if (!u->urb)
goto out_of_memory;
- u->urb->transfer_buffer = ep->syncbuf + i * 4;
+ usb_fill_int_urb(u->urb, NULL, ep->pipe, ep->syncbuf + i * 4, 4,
+snd_complete_urb, u, ep->syncinterval + 1);
+
u->urb->transfer_dma = ep->sync_dma + i * 4;
- u->urb->transfer_buffer_length = 4;
- u->urb->pipe = ep->pipe;
u->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
u->urb->number_of_packets = 1;
- u->urb->interval = 1 << ep->syncinterval;
- u->urb->context = u;
- u->urb->complete = snd_complete_urb;
}
ep->nurbs = SYNC_URBS;
--
2.17.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
Hi, Tony Lindgren writes: > * Felipe Balbi [180619 01:23]: >> This is a direct consequence of not paying attention to the order of >> things. If driver were to assume that pm_domain->activate() would do the >> right thing for the device -- meaning that probe would run with an >> active device --, then we wouldn't need that pm_runtime_get() call on >> probe at all. Rather we would follow the sequence: >> >> pm_runtime_forbid() >> pm_runtime_set_active() >> pm_runtime_enable() >> >> /* do your probe routine */ >> >> pm_runtime_put_noidle() >> >> Then you remove you would need to call pm_runtime_get_noresume() to >> balance out the pm_runtime_put_noidle() there. > > How about let's create some prettier interface for the above runtime PM > trickery? > > How about something like pm_runtime_init_enabled() for the above > sequence? > > It might be then able to do the trick even if activate is not > implemented.. > > Right now it has the feeling of "oh well we can't get runtime PM to > work so let's bypass it with activate call and then trick runtime PM > to start in enabled mode" :) no strong feelings about that either way. It's only 3 lines of code anyway. I feel like that's a minor cosmetic change. >> (If you need to know why the pm_runtime_put_noidle(), remember that >> pm_runtime_set_active() increments the usage counter, so >> pm_runtime_put_noidle is basically allowing pm_runtime to happen as soon >> as userspace writes "auto" to /sys//power/control) > > I wonder if we could also remove the need for drivers to call > pm_runtime_putnoidle() at the end of the probe? If we had > pm_runtime_init_enabled() implemented. probably not. We want to block runtime pm during probe, until the device is fully initialized, the only way to do that is to increment rpm usage counter. -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
Hi, Adding Rafael and linux-pm to Cc as well. * Felipe Balbi [180619 01:23]: > This is a direct consequence of not paying attention to the order of > things. If driver were to assume that pm_domain->activate() would do the > right thing for the device -- meaning that probe would run with an > active device --, then we wouldn't need that pm_runtime_get() call on > probe at all. Rather we would follow the sequence: > > pm_runtime_forbid() > pm_runtime_set_active() > pm_runtime_enable() > > /* do your probe routine */ > > pm_runtime_put_noidle() > > Then you remove you would need to call pm_runtime_get_noresume() to > balance out the pm_runtime_put_noidle() there. How about let's create some prettier interface for the above runtime PM trickery? How about something like pm_runtime_init_enabled() for the above sequence? It might be then able to do the trick even if activate is not implemented.. Right now it has the feeling of "oh well we can't get runtime PM to work so let's bypass it with activate call and then trick runtime PM to start in enabled mode" :) > Anyway, with an assumption like this, after all platform_devices are > converted over, the assumption can be moved into the bus code and, low > and behold, to enable runtime pm for your driver, all you have to is > implement your callbacks and add pm_runtime_put_noidle() to probe and > pm_runtime_get_noresume() to remove (apart from, of course, making sure > the device isn't allowed to runtime_suspend when it's actually busy). > > Do you see the end goal? It certainly would be nice to make runtime PM generic for drivers :) > (If you need to know why the pm_runtime_put_noidle(), remember that > pm_runtime_set_active() increments the usage counter, so > pm_runtime_put_noidle is basically allowing pm_runtime to happen as soon > as userspace writes "auto" to /sys//power/control) I wonder if we could also remove the need for drivers to call pm_runtime_putnoidle() at the end of the probe? If we had pm_runtime_init_enabled() implemented. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: ftdi_sio: Add MTP NVM support
Overall nvmem side looks good!
Minor nits below.
On 14/06/18 21:08, Loic Poulain wrote:
Most of FTDI's devices have an EEPROM which records FTDI devices
configuration setting (e.g. the VID, PID, I/O config...) and user
data. FT230R chip integrates a 128-byte eeprom, FT230X a 2048-byte
eeprom...
This patch adds support for FTDI EEPROM read/write via USB control
transfers and register a new nvm device to the nvmem core.
This permits to expose the eeprom as a sysfs file, allowing userspace
to read/modify FTDI configuration and its user data without having to
rely on a specific userspace USB driver.
Moreover, any upcoming new tentative to add CBUS GPIO support could
integrate CBUS EEPROM configuration reading in order to determine
which of the CBUS pins are available as GPIO.
Signed-off-by: Loic Poulain
---
drivers/usb/serial/Kconfig| 11 +
drivers/usb/serial/ftdi_sio.c | 108 ++
drivers/usb/serial/ftdi_sio.h | 28 +++
3 files changed, 147 insertions(+)
diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
index 533f127..2dd2f5d 100644
--- a/drivers/usb/serial/Kconfig
+++ b/drivers/usb/serial/Kconfig
@@ -181,6 +181,17 @@ config USB_SERIAL_FTDI_SIO
To compile this driver as a module, choose M here: the
module will be called ftdi_sio.
+config USB_SERIAL_FTDI_SIO_NVMEM
+bool "FTDI MTP non-volatile memory support"
+ depends on USB_SERIAL_FTDI_SIO
Looks inconsistent tab and spaces here.
+select NVMEM
+default y
??
Does that mean all the FTDIs have EEPROM?
+help
+ Say yes here to add support for the MTP non-volatile memory
+ present on FTDI. Most of FTDI's devices have an EEPROM which
+ records FTDI device's configuration setting as well as user
+ data.
+
config USB_SERIAL_VISOR
tristate "USB Handspring Visor / Palm m50x / Sony Clie Driver"
help
diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 7ea221d..03c9c75 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -40,6 +40,7 @@
#include
#include
#include
+#include
#include "ftdi_sio.h"
#include "ftdi_sio_ids.h"
@@ -73,6 +74,8 @@ struct ftdi_private {
unsigned int latency; /* latency setting in use */
unsigned short max_packet_size;
struct mutex cfg_lock; /* Avoid mess by parallel calls of config
ioctl() and change_speed() */
+
+ struct nvmem_device *eeprom;
};
/* struct ftdi_sio_quirk is used by devices requiring special attention. */
@@ -1529,6 +1532,104 @@ static int get_lsr_info(struct usb_serial_port *port,
return 0;
}
+#if IS_ENABLED(CONFIG_USB_SERIAL_FTDI_SIO_NVMEM)
I think only ifdef should be Okay here as this Kconfig is just bool
+
...
+static int register_eeprom(struct usb_serial_port *port)
+{
+ struct ftdi_private *priv = usb_get_serial_port_data(port);
+ struct nvmem_config nvmconf = {};
+
+ switch (priv->chip_type) {
+ case FTX:
+ nvmconf.size = 2048;
+ break;
+ case FT232RL:
+ nvmconf.size = 128;
+ break;
+ default:
+ return 0;
+ }
+
+ nvmconf.word_size = 2;
+ nvmconf.stride = 2;
+ nvmconf.read_only = false;
+ nvmconf.priv = port;
+ nvmconf.dev = >dev;
+ nvmconf.reg_read = read_eeprom;
+ nvmconf.reg_write = write_eeprom;
+ nvmconf.owner = THIS_MODULE;
+
+ priv->eeprom = nvmem_register();
+ if (IS_ERR(priv->eeprom)) {
+ priv->eeprom = NULL;
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static void unregister_eeprom(struct usb_serial_port *port)
+{
+ struct ftdi_private *priv = usb_get_serial_port_data(port);
+
+ if (priv->eeprom)
+ nvmem_unregister(priv->eeprom);
+}
+
+#endif /* CONFIG_USB_SERIAL_FTDI_SIO_NVMEM */
/* Determine type of FTDI chip based on USB config and descriptor. */
static void ftdi_determine_type(struct usb_serial_port *port)
@@ -1814,6 +1915,10 @@ static int ftdi_sio_port_probe(struct usb_serial_port
*port)
priv->latency = 16;
write_latency_timer(port);
create_sysfs_attrs(port);
+#if IS_ENABLED(CONFIG_USB_SERIAL_FTDI_SIO_NVMEM)
+ register_eeprom(port);
Users will be clueless if the register_eeprom fails here.
+#endif
+
return 0;
}
@@ -1931,6 +2036,9 @@ static int ftdi_sio_port_remove(struct usb_serial_port *port)
{
struct ftdi_private *priv = usb_get_serial_port_data(port);
+#if IS_ENABLED(CONFIG_USB_SERIAL_FTDI_SIO_NVMEM)
+ unregister_eeprom(port);
+#endif
remove_sysfs_attrs(port);
kfree(priv);
thanks,
srini
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info
Re: [PATCH] USB: serial: ftdi_sio: Add MTP NVM support
Hi Johan, Srini, On 19 June 2018 at 15:06, Johan Hovold wrote: > On Tue, Jun 19, 2018 at 02:32:11PM +0200, Loic Poulain wrote: >> Hi Johan, Srini, >> >> On 18 June 2018 at 11:47, Srinivas Kandagatla >> wrote: >> > On 18/06/18 09:46, Johan Hovold wrote: > >> >> I'm not necessarily against the idea, but nvmem core needs to be fixed >> >> so that it can handle hotplugging before this can be considered for >> >> merging. >> >> >> >> Right now it just returns -EBUSY from nvmem_unregister(), which results >> >> in all kinds of memory leaks, use-after-frees and crashes when user >> >> space holds the character device open while the device is being >> >> unplugged. >> >> Correct me if I'm wrong, but nvmem is just exposed to userspace as a >> simple sysfs device attribute (nvmem), removing a device and its attribute(s) >> dynamically is well managed by sysfs, even if userspace has file open. > > My bad, I misremembered what interface nvmem was using and only saw the > deregistration refusal in nvmem_unregister() during a quick check. > >> The only risk here is when a kernel internal consumer still has a reference >> to >> the nvmem device on removal, which is not the case in our context. > > Right. > >> > I can also suggest you to try devm_nvmem_register(). >> >> Yes, I'm going to send a v2. > > I'll take a closer look soonish too. I changed my mind here, indeed, in order to avoid any potential use-after-free, we have to manually unregister/release the nvmem device before freeing of the private port structure (used as nvmem context). So I will not send a V2 with nvmem devm usage. You can consider this V1 for review. Regards, Loic -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/9] ALSA: usb-audio: use usb_fill_int_urb()
Hello! On 6/20/2018 12:55 AM, Sebastian Andrzej Siewior wrote: Using usb_fill_int_urb() helps to find code which initializes an URB. A grep for members of the struct (like ->complete) reveal lots of other things, too. data_ep_set_params() additionally sets urb->transfer_buffer_length which was not the case earlier. data_ep_set_params() and data_ep_set_params() These 2 are the same function? ensure that syncinterval is within the allowed range on HS/SS. The interval value seems to come from snd_usb_parse_datainterval() which is bInterval - 1 and only in the rage 1 … 4. So in order to keep the magic wokring I pass datainterval + 1. Working. Cc: Jaroslav Kysela Cc: Takashi Iwai Signed-off-by: Sebastian Andrzej Siewior [...] MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
