I was trying to get rid of the in in_softirq() in ezusb_req_ctx_wait()
within the orinoco usb driver,
drivers/net/wireless/intersil/orinoco/orinoco_usb.c. A small snippet:
| static void ezusb_req_ctx_wait(struct ezusb_priv *upriv,
| struct request_context *ctx)
…
| if (in_softirq()) {
| /* If we get called from a timer, timeout timers don't
| * get the chance to run themselves. So we make sure
| * that we don't sleep for ever */
| int msecs = DEF_TIMEOUT * (1000 / HZ);
|
| while (!try_wait_for_completion(&ctx->done) && msecs--)
| udelay(1000);
| } else {
| wait_for_completion(&ctx->done);
…
| }
This is broken. The EHCI and XHCI HCD will complete the URB in
BH/tasklet. Should we ever get here in_softirq() then we will spin
here/wait here until the timeout passes because the tasklet won't be
able to run. OHCI/UHCI HCDs still complete in hard-IRQ so it would work
here.
Is it possible to end up here in softirq context or is this a relic?
Well I have no hardware but I see this:
orinoco_set_monitor_channel() [I assume that this is fully preemtible]
-> orinoco_lock() [this should point to ezusb_lock_irqsave() which
does spin_lock_bh(lock), so from here on
in_softirq() returns true]
-> hw->ops->cmd_wait() [-> ezusb_docmd_wait()]
-> ezusb_alloc_ctx() [ sets ctx->in_rid to EZUSB_RID_ACK/0x0710 ]
-> ezusb_access_ltv()
-> if (ctx->in_rid)
-> ezusb_req_ctx_wait(upriv, ctx);
-> ctx->state should be EZUSB_CTX_REQ_COMPLETE so we end up in
the while loop above. So we udelay() 3 * 1000 * 1ms = 3sec.
-> Then ezusb_access_ltv() should return with an error due to
timeout.
This isn't limited to exotic features like monitor mode. orinoco_open()
does orinoco_lock() followed by orinoco_hw_program_rids() which in the
end invokes ezusb_write_ltv(,, EZUSB_RID_ACK) which is non-zero and also
would block (ezusb_xmit() would use 0 as the last argument so it won't
block).
I don't see how this driver can work on EHCI/XHCI HCD as of today.
The driver is an orphan since commit
3a59babbee409 ("orinoco: update status in MAINTAINERS")
which is ten years ago. If I replace in_softirq() with a `may_sleep'
argument then it is still broken.
Should it be removed?
Sebastian
On Fri, Oct 02, 2020 at 12:35:17PM +0200, Sebastian Andrzej Siewior wrote:
> I was trying to get rid of the in in_softirq() in ezusb_req_ctx_wait()
> within the orinoco usb driver,
> drivers/net/wireless/intersil/orinoco/orinoco_usb.c. A small snippet:
>
> | static void ezusb_req_ctx_wait(struct ezusb_priv *upriv,
> | struct request_context *ctx)
> …
> | if (in_softirq()) {
> | /* If we get called from a timer, timeout timers don't
> | * get the chance to run themselves. So we make sure
> | * that we don't sleep for ever */
> | int msecs = DEF_TIMEOUT * (1000 / HZ);
> |
> | while (!try_wait_for_completion(&ctx->done) && msecs--)
> | udelay(1000);
> | } else {
> | wait_for_completion(&ctx->done);
> …
> | }
>
> This is broken. The EHCI and XHCI HCD will complete the URB in
> BH/tasklet. Should we ever get here in_softirq() then we will spin
> here/wait here until the timeout passes because the tasklet won't be
> able to run. OHCI/UHCI HCDs still complete in hard-IRQ so it would work
> here.
>
> Is it possible to end up here in softirq context or is this a relic?
I think it's a relic of where USB host controllers completed their urbs
in hard-irq mode. The BH/tasklet change is a pretty recent change.
> Well I have no hardware but I see this:
>
> orinoco_set_monitor_channel() [I assume that this is fully preemtible]
> -> orinoco_lock() [this should point to ezusb_lock_irqsave() which
> does spin_lock_bh(lock), so from here on
> in_softirq() returns true]
> -> hw->ops->cmd_wait() [-> ezusb_docmd_wait()]
> -> ezusb_alloc_ctx() [ sets ctx->in_rid to EZUSB_RID_ACK/0x0710 ]
> -> ezusb_access_ltv()
> -> if (ctx->in_rid)
> -> ezusb_req_ctx_wait(upriv, ctx);
> -> ctx->state should be EZUSB_CTX_REQ_COMPLETE so we end up in
> the while loop above. So we udelay() 3 * 1000 * 1ms = 3sec.
> -> Then ezusb_access_ltv() should return with an error due to
> timeout.
>
> This isn't limited to exotic features like monitor mode. orinoco_open()
> does orinoco_lock() followed by orinoco_hw_program_rids() which in the
> end invokes ezusb_write_ltv(,, EZUSB_RID_ACK) which is non-zero and also
> would block (ezusb_xmit() would use 0 as the last argument so it won't
> block).
>
> I don't see how this driver can work on EHCI/XHCI HCD as of today.
> The driver is an orphan since commit
> 3a59babbee409 ("orinoco: update status in MAINTAINERS")
>
> which is ten years ago. If I replace in_softirq() with a `may_sleep'
> argument then it is still broken.
> Should it be removed?
We can move it out to drivers/staging/ and then drop it to see if anyone
complains that they have the device and is willing to test any changes.
thanks,
greg k-h
On 2020-10-02 13:37:25 [+0200], Greg Kroah-Hartman wrote:
> > Is it possible to end up here in softirq context or is this a relic?
>
> I think it's a relic of where USB host controllers completed their urbs
> in hard-irq mode. The BH/tasklet change is a pretty recent change.
But the BH thingy for HCDs went in v3.12 for EHCI. XHCI was v5.5. My
guess would be that people using orinoco USB are on EHCI :)
> > Should it be removed?
>
> We can move it out to drivers/staging/ and then drop it to see if anyone
> complains that they have the device and is willing to test any changes.
Not sure moving is easy since it depends on other files in that folder.
USB is one interface next to PCI for instance. Unless you meant to move
the whole driver including all interfaces.
I was suggesting to remove the USB bits.
> thanks,
>
> greg k-h
Sebastian
On Fri, Oct 02, 2020 at 01:53:58PM +0200, Sebastian Andrzej Siewior wrote:
> On 2020-10-02 13:37:25 [+0200], Greg Kroah-Hartman wrote:
> > > Is it possible to end up here in softirq context or is this a relic?
> >
> > I think it's a relic of where USB host controllers completed their urbs
> > in hard-irq mode. The BH/tasklet change is a pretty recent change.
>
> But the BH thingy for HCDs went in v3.12 for EHCI. XHCI was v5.5. My
> guess would be that people using orinoco USB are on EHCI :)
USB 3 systems run XHCI, which has a USB 2 controller in it, so these
types of things might not have been noticed yet. Who knows :)
> > > Should it be removed?
> >
> > We can move it out to drivers/staging/ and then drop it to see if anyone
> > complains that they have the device and is willing to test any changes.
>
> Not sure moving is easy since it depends on other files in that folder.
> USB is one interface next to PCI for instance. Unless you meant to move
> the whole driver including all interfaces.
> I was suggesting to remove the USB bits.
I forgot this was tied into other code, sorry. I don't know what to
suggest other than maybe try to fix it up the best that you can, and
let's see if anyone notices...
thanks,
greg k-h
Greg Kroah-Hartman <[email protected]> writes:
> On Fri, Oct 02, 2020 at 01:53:58PM +0200, Sebastian Andrzej Siewior wrote:
>> On 2020-10-02 13:37:25 [+0200], Greg Kroah-Hartman wrote:
>> > > Is it possible to end up here in softirq context or is this a relic?
>> >
>> > I think it's a relic of where USB host controllers completed their urbs
>> > in hard-irq mode. The BH/tasklet change is a pretty recent change.
>>
>> But the BH thingy for HCDs went in v3.12 for EHCI. XHCI was v5.5. My
>> guess would be that people using orinoco USB are on EHCI :)
>
> USB 3 systems run XHCI, which has a USB 2 controller in it, so these
> types of things might not have been noticed yet. Who knows :)
>
>> > > Should it be removed?
>> >
>> > We can move it out to drivers/staging/ and then drop it to see if anyone
>> > complains that they have the device and is willing to test any changes.
>>
>> Not sure moving is easy since it depends on other files in that folder.
>> USB is one interface next to PCI for instance. Unless you meant to move
>> the whole driver including all interfaces.
>> I was suggesting to remove the USB bits.
>
> I forgot this was tied into other code, sorry. I don't know what to
> suggest other than maybe try to fix it up the best that you can, and
> let's see if anyone notices...
That's what I would suggest as well.
These drivers for ancient hardware are tricky. Even if there doesn't
seem to be any users on the driver sometimes people pop up reporting
that it's still usable. We had that recently with one another wireless
driver (forgot the name already).
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
+ Jes
On 10/5/2020 4:12 PM, Kalle Valo wrote:
> Greg Kroah-Hartman <[email protected]> writes:
>
>> On Fri, Oct 02, 2020 at 01:53:58PM +0200, Sebastian Andrzej Siewior wrote:
>>> On 2020-10-02 13:37:25 [+0200], Greg Kroah-Hartman wrote:
>>>>> Is it possible to end up here in softirq context or is this a relic?
>>>>
>>>> I think it's a relic of where USB host controllers completed their urbs
>>>> in hard-irq mode. The BH/tasklet change is a pretty recent change.
>>>
>>> But the BH thingy for HCDs went in v3.12 for EHCI. XHCI was v5.5. My
>>> guess would be that people using orinoco USB are on EHCI :)
>>
>> USB 3 systems run XHCI, which has a USB 2 controller in it, so these
>> types of things might not have been noticed yet. Who knows :)
>>
>>>>> Should it be removed?
>>>>
>>>> We can move it out to drivers/staging/ and then drop it to see if anyone
>>>> complains that they have the device and is willing to test any changes.
>>>
>>> Not sure moving is easy since it depends on other files in that folder.
>>> USB is one interface next to PCI for instance. Unless you meant to move
>>> the whole driver including all interfaces.
>>> I was suggesting to remove the USB bits.
>>
>> I forgot this was tied into other code, sorry. I don't know what to
>> suggest other than maybe try to fix it up the best that you can, and
>> let's see if anyone notices...
>
> That's what I would suggest as well.
>
> These drivers for ancient hardware are tricky. Even if there doesn't
> seem to be any users on the driver sometimes people pop up reporting
> that it's still usable. We had that recently with one another wireless
> driver (forgot the name already).
Quite a while ago I shipped an orinoco dongle to Jes Sorensen which he
wanted to use for some intern project if I recall correctly. Guess that
idea did not fly yet.
Regards,
Arend
On Tue, 2020-10-06 at 08:40 -0400, Jes Sorensen wrote:
> On 10/6/20 3:45 AM, Arend Van Spriel wrote:
> > + Jes
> >
> > On 10/5/2020 4:12 PM, Kalle Valo wrote:
> > > Greg Kroah-Hartman <[email protected]> writes:
> > >
> > > > On Fri, Oct 02, 2020 at 01:53:58PM +0200, Sebastian Andrzej
> > > > Siewior
> > > > wrote:
> > > > > On 2020-10-02 13:37:25 [+0200], Greg Kroah-Hartman wrote:
> > > > > > > Is it possible to end up here in softirq context or is
> > > > > > > this a relic?
> > > > > >
> > > > > > I think it's a relic of where USB host controllers
> > > > > > completed their
> > > > > > urbs
> > > > > > in hard-irq mode. The BH/tasklet change is a pretty recent
> > > > > > change.
> > > > >
> > > > > But the BH thingy for HCDs went in v3.12 for EHCI. XHCI was
> > > > > v5.5. My
> > > > > guess would be that people using orinoco USB are on EHCI :)
> > > >
> > > > USB 3 systems run XHCI, which has a USB 2 controller in it, so
> > > > these
> > > > types of things might not have been noticed yet. Who knows :)
> > > >
> > > > > > > Should it be removed?
> > > > > >
> > > > > > We can move it out to drivers/staging/ and then drop it to
> > > > > > see if
> > > > > > anyone
> > > > > > complains that they have the device and is willing to test
> > > > > > any
> > > > > > changes.
> > > > >
> > > > > Not sure moving is easy since it depends on other files in
> > > > > that folder.
> > > > > USB is one interface next to PCI for instance. Unless you
> > > > > meant to move
> > > > > the whole driver including all interfaces.
> > > > > I was suggesting to remove the USB bits.
> > > >
> > > > I forgot this was tied into other code, sorry. I don't know
> > > > what to
> > > > suggest other than maybe try to fix it up the best that you
> > > > can, and
> > > > let's see if anyone notices...
> > >
> > > That's what I would suggest as well.
> > >
> > > These drivers for ancient hardware are tricky. Even if there
> > > doesn't
> > > seem to be any users on the driver sometimes people pop up
> > > reporting
> > > that it's still usable. We had that recently with one another
> > > wireless
> > > driver (forgot the name already).
> >
> > Quite a while ago I shipped an orinoco dongle to Jes Sorensen which
> > he
> > wanted to use for some intern project if I recall correctly. Guess
> > that
> > idea did not fly yet.
>
> I had an outreachy intern who worked on some of it, so I shipped all
> my
> Orinoco hardware to her. We never made as much progress as I had
> hoped,
> and I haven't had time to work on it since.
If anyone wants orinoco_usb, I think I still have one or two. I may
also have original orinoco hardware (PCMCIA) if anyone wants it.
Dan