Re: [PATCH 00/16] remove eight obsolete architectures

2018-03-15 Thread Hannes Reinecke
On 03/15/2018 10:42 AM, David Howells wrote:
> Do we have anything left that still implements NOMMU?
> 
RISC-V ?
(evil grin :-)

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
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 11/11] usb/gadget: Make it again possible to enable the legacy drivers without enabling USB_ETH

2017-11-02 Thread Hannes Reinecke
On 10/31/2017 07:03 PM, Bart Van Assche wrote:
> Considerable time ago the legacy gadget menu was added inside the
> USB_ETH choice. I think this was a mistake and that the legacy
> gadget menu should have been added after "endchoice" instead of
> before. Hence this patch.
> 
> Fixes: commit 8443f2d2b778 ("usb: gadget: Gadget directory cleanup - group 
> legacy gadgets")
> Signed-off-by: Bart Van Assche 
> Cc: Nicholas Bellinger 
> Cc: Andrzej Pietrasiewicz 
> Cc: Felipe Balbi 
> Cc: linux-usb@vger.kernel.org
> ---
>  drivers/usb/gadget/Kconfig|  4 ++--
>  drivers/usb/gadget/legacy/Kconfig | 10 ++
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index 31cce7805eb2..0a19a76645ad 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -508,8 +508,8 @@ choice
> controller, and the relevant drivers for each function declared
> by the device.
>  
> -source "drivers/usb/gadget/legacy/Kconfig"
> -
>  endchoice
>  
> +source "drivers/usb/gadget/legacy/Kconfig"
> +
>  endif # USB_GADGET
> diff --git a/drivers/usb/gadget/legacy/Kconfig 
> b/drivers/usb/gadget/legacy/Kconfig
> index a12fb459dbd9..9570bbeced4f 100644
> --- a/drivers/usb/gadget/legacy/Kconfig
> +++ b/drivers/usb/gadget/legacy/Kconfig
> @@ -13,6 +13,14 @@
>  # both kinds of controller can also support "USB On-the-Go" (CONFIG_USB_OTG).
>  #
>  
> +menuconfig USB_GADGET_LEGACY
> + bool "Legacy USB Gadget Support"
> + help
> +Legacy USB gadgets are USB gadgets that do not use the USB gadget
> +configfs interface.
> +
> +if USB_GADGET_LEGACY
> +
>  config USB_ZERO
>   tristate "Gadget Zero (DEVELOPMENT)"
>   select USB_LIBCOMPOSITE
> @@ -490,3 +498,5 @@ config USB_G_WEBCAM
>  
> Say "y" to link the driver statically, or "m" to build a
> dynamically linked module called "g_webcam".
> +
> +endif
> 
Reviewed-by: Hannes Reinecke 

Cheers,

Hannes

-- 
Dr. Hannes ReineckezSeries & Storage
h...@suse.com  +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
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] Remove deprecated IRQF_DISABLED flag entirely

2015-03-05 Thread Hannes Reinecke
On 03/05/2015 01:59 PM, Valentin Rothberg wrote:
> The IRQF_DISABLED is a NOOP and has been scheduled for removal since
> Linux v2.6.36 by commit 6932bf37bed4 ("genirq: Remove IRQF_DISABLED from
> core code").
> 
> According to commit e58aa3d2d0cc ("genirq: Run irq handlers with
> interrupts disabled") running IRQ handlers with interrupts enabled can
> cause stack overflows when the interrupt line of the issuing device is
> still active.
> 
> This patch ends the grace period for IRQF_DISABLED (i.e., SA_INTERRUPT
> in older versions of Linux) and removes the definition and all remaining
> usages of this flag.
> 
> Signed-off-by: Valentin Rothberg 
> ---
> The bigger hunk in Documentation/scsi/ncr53c8xx.txt is removed entirely
> as IRQF_DISABLED is gone now; the usage in older kernel versions
> (including the old SA_INTERRUPT flag) should be discouraged.  The
> trouble of using IRQF_SHARED is a general problem and not specific to
> any driver.
> 
> I left the reference in Documentation/PCI/MSI-HOWTO.txt untouched since
> it has already been removed in linux-next by commit b0e1ee8e1405
> ("MSI-HOWTO.txt: remove reference on IRQF_DISABLED").
> 
> All remaining references are changelogs that I suggest to keep.

While you're at it: having '0x0' as a value for the irq flags looks
a bit silly, and makes you wonder what the parameter is for.

I would rather like to have

#define IRQF_NONE 0x0

and use it for these cases.
That way the scope of that parameter is clear.

Cheers,

Hannes
-- 
Dr. Hannes ReineckezSeries & Storage
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
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 v3 1/4] scsi: add ability to adjust module reference for scsi host

2015-01-12 Thread Hannes Reinecke
On 01/11/2015 02:50 PM, Akinobu Mita wrote:
> While accessing a scsi_device, the use count of the underlying LLDD module
> is incremented.  The module reference is retrieved through .module field of
> struct scsi_host_template.
> 
> This mapping between scsi_device and underlying LLDD module works well
> except ufs, unusual usb storage drivers, and sub drivers for esp_scsi.
> These drivers consist with core driver and actual LLDDs, and
> scsi_host_template is defined in the core driver.  So the actual LLDDs can
> be unloaded even if the scsi_device is being accessed.
> 
> This adds .module field in struct Scsi_Host and let the module reference
> be retrieved though it instead of struct scsi_host_template.  This allows
> the actual LLDDs adjust module reference.
> 
> Signed-off-by: Akinobu Mita 
> Cc: Vinayak Holikatti 
> Cc: Dolev Raviv 
> Cc: Sujit Reddy Thumma 
> Cc: Subhash Jadavani 
> Cc: Christoph Hellwig 
> Cc: "James E.J. Bottomley" 
> Cc: Matthew Dharm 
> Cc: Greg Kroah-Hartman 
> Cc: Alan Stern 
> Cc: linux-usb@vger.kernel.org
> Cc: usb-stor...@lists.one-eyed-alien.net
> Cc: linux-s...@vger.kernel.org

Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckezSeries & Storage
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
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] SCSI: fix regression in scsi_send_eh_cmnd()

2014-11-21 Thread Hannes Reinecke
On 11/21/2014 04:44 PM, Alan Stern wrote:
> Commit ac61d1955934 (scsi: set correct completion code in
> scsi_send_eh_cmnd()) introduced a bug.  It changed the stored return
> value from a queuecommand call, but it didn't take into account that
> the return value was used again later on.  This patch fixes the bug by
> changing the later usage.
> 
> There is a big comment in the middle of scsi_send_eh_cmnd() which
> does a good job of explaining how the routine works.  But it mentions
> a "rtn = FAILURE" value that doesn't exist in the code.  This patch
> adjusts the code to match the comment (I assume the comment is right
> and the code is wrong).
> 
> This fixes Bugzilla #88341.
> 
> Signed-off-by: Alan Stern 
> Reported-by: Андрей Аладьев 
> Tested-by: Андрей Аладьев 
> Fixes: ac61d19559349e205dad7b5122b281419aa74a82
> CC: Hannes Reinecke 
> CC: 
> 
That is correct.

Acked-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
--
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 0/3] Fix USB deadlock caused by SCSI error handling

2014-04-10 Thread Hannes Reinecke
On 04/10/2014 10:36 PM, James Bottomley wrote:
> On Thu, 2014-04-10 at 19:52 +0200, Hannes Reinecke wrote:
>> On 04/10/2014 05:31 PM, Alan Stern wrote:
>>> On Thu, 10 Apr 2014, Hannes Reinecke wrote:
>>>
>>>> On 04/10/2014 12:58 PM, Andreas Reis wrote:
>>>>> That patch appears to work in preventing the crashes, judged on one
>>>>> repeated appearance of the bug.
>>>>>
>>>>> dmesg had the usual
>>>>> [  215.229903] usb 4-2: usb_disable_lpm called, do nothing
>>>>> [  215.336941] usb 4-2: reset SuperSpeed USB device number 3 using
>>>>> xhci_hcd
>>>>> [  215.350296] xhci_hcd :00:14.0: xHCI xhci_drop_endpoint called
>>>>> with disabled ep 880427b829c0
>>>>> [  215.350305] xhci_hcd :00:14.0: xHCI xhci_drop_endpoint called
>>>>> with disabled ep 880427b82a08
>>>>> [  215.350621] usb 4-2: usb_enable_lpm called, do nothing
>>>>>
>>>>> repeated five times, followed by one
>>>>> [  282.795801] sd 8:0:0:0: Device offlined - not ready after error
>>>>> recovery
>>>>>
>>>>> and then as often as something tried to read from it:
>>>>> [  295.585472] sd 8:0:0:0: rejecting I/O to offline device
>>>>>
>>>>> The stick could then be properly un- and remounted (the latter if it
>>>>> had been physically replugged) without issue � for the bug to
>>>>> reoccur after one to three minutes. I tried this three times, no
>>>>> dmesg difference except the ep addresses varied on two of that.
>>>>>
>>>> Was this just that patch you've tested with or the entire patch series?
>>>>
>>>> If the latter, Alan, is this the expected outcome?
>>>
>>> Yes, it is.  The same thing should happen with the entire patch series.
>>>
>>>> I would've thought the error recover should _not_ run into
>>>> offlining devices here, but rather the device should be recovered
>>>> eventually.
>>>
>>> The command times out, it is aborted, and the command is retried.  The
>>> same thing happens, and we repeat five times.  Eventually the SCSI core
>>> gives up and declares the device to be offline.
>>>
>> Hmm. Ok. If you are fine with it who am I to argue here.
>> James, shall I resent the patch series?
> 
> You mean the one patch?  No, it's OK, I have it.
> 
> It's still not complete, though, as I've said a couple of times.  The
> problem is that we have abort memory on any eh command as well, which
> this doesn't fix.
> 
> The scenario is abort command, set flag, abort completes, send TUR, TUR
> doesn't return, so we now try to abort the TUR, but scsi_abort_eh_cmnd()
> will skip the abort because the flag is set and move straight to reset.
> 
> The fix is this, I can just add it as well.
> 
> James
> 
> ---
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 771c16b..7516e2c 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -920,6 +920,7 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct 
> scsi_eh_save *ses,
>   ses->prot_op = scmd->prot_op;
>  
>   scmd->prot_op = SCSI_PROT_NORMAL;
> + scmd->eh_eflags = 0;
>   scmd->cmnd = ses->eh_cmnd;
>   memset(scmd->cmnd, 0, BLK_MAX_CDB);
>   memset(&scmd->sdb, 0, sizeof(scmd->sdb));
> 
> 
Oh yes, that is correct.

Acked-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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 0/3] Fix USB deadlock caused by SCSI error handling

2014-04-10 Thread Hannes Reinecke

On 04/10/2014 05:31 PM, Alan Stern wrote:

On Thu, 10 Apr 2014, Hannes Reinecke wrote:


On 04/10/2014 12:58 PM, Andreas Reis wrote:

That patch appears to work in preventing the crashes, judged on one
repeated appearance of the bug.

dmesg had the usual
[  215.229903] usb 4-2: usb_disable_lpm called, do nothing
[  215.336941] usb 4-2: reset SuperSpeed USB device number 3 using
xhci_hcd
[  215.350296] xhci_hcd :00:14.0: xHCI xhci_drop_endpoint called
with disabled ep 880427b829c0
[  215.350305] xhci_hcd :00:14.0: xHCI xhci_drop_endpoint called
with disabled ep 880427b82a08
[  215.350621] usb 4-2: usb_enable_lpm called, do nothing

repeated five times, followed by one
[  282.795801] sd 8:0:0:0: Device offlined - not ready after error
recovery

and then as often as something tried to read from it:
[  295.585472] sd 8:0:0:0: rejecting I/O to offline device

The stick could then be properly un- and remounted (the latter if it
had been physically replugged) without issue � for the bug to
reoccur after one to three minutes. I tried this three times, no
dmesg difference except the ep addresses varied on two of that.


Was this just that patch you've tested with or the entire patch series?

If the latter, Alan, is this the expected outcome?


Yes, it is.  The same thing should happen with the entire patch series.


I would've thought the error recover should _not_ run into
offlining devices here, but rather the device should be recovered
eventually.


The command times out, it is aborted, and the command is retried.  The
same thing happens, and we repeat five times.  Eventually the SCSI core
gives up and declares the device to be offline.


Hmm. Ok. If you are fine with it who am I to argue here.
James, shall I resent the patch series?

Cheers,

Hannes

--
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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 0/3] Fix USB deadlock caused by SCSI error handling

2014-04-10 Thread Hannes Reinecke
On 04/10/2014 02:26 PM, Andreas Reis wrote:
> Only your 0/3 patch to which Alan linked, along with two other
> patches by Mathias Nyman ("disable usb3 on intel hosts" and "disable
> all lpm related control transfers", one of which is the source of
> the "do nothing"s).
> 
> I'll revert the latter two and apply the rest of the set. Which I'm
> guessing currently consists of said 0/3 patch —
> http://www.spinics.net/lists/linux-scsi/msg73502.html
> — plus 2/3 and 3/3?
> 
Yes, that is correct.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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 0/3] Fix USB deadlock caused by SCSI error handling

2014-04-10 Thread Hannes Reinecke
On 04/10/2014 12:58 PM, Andreas Reis wrote:
> That patch appears to work in preventing the crashes, judged on one
> repeated appearance of the bug.
> 
> dmesg had the usual
> [  215.229903] usb 4-2: usb_disable_lpm called, do nothing
> [  215.336941] usb 4-2: reset SuperSpeed USB device number 3 using
> xhci_hcd
> [  215.350296] xhci_hcd :00:14.0: xHCI xhci_drop_endpoint called
> with disabled ep 880427b829c0
> [  215.350305] xhci_hcd :00:14.0: xHCI xhci_drop_endpoint called
> with disabled ep 880427b82a08
> [  215.350621] usb 4-2: usb_enable_lpm called, do nothing
> 
> repeated five times, followed by one
> [  282.795801] sd 8:0:0:0: Device offlined - not ready after error
> recovery
> 
> and then as often as something tried to read from it:
> [  295.585472] sd 8:0:0:0: rejecting I/O to offline device
> 
> The stick could then be properly un- and remounted (the latter if it
> had been physically replugged) without issue — for the bug to
> reoccur after one to three minutes. I tried this three times, no
> dmesg difference except the ep addresses varied on two of that.
> 
Was this just that patch you've tested with or the entire patch series?

If the latter, Alan, is this the expected outcome?
I would've thought the error recover should _not_ run into
offlining devices here, but rather the device should be recovered
eventually.

Andreas, can you test with the entire patch series and enable
'scsi_logging_level -s -E 5' prior to running the tests?

THX.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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 0/3] Fix USB deadlock caused by SCSI error handling

2014-04-09 Thread Hannes Reinecke

On 04/07/2014 05:26 PM, Alan Stern wrote:

On Wed, 2 Apr 2014, Hannes Reinecke wrote:


On 04/01/2014 11:28 PM, Alan Stern wrote:

On Tue, 1 Apr 2014, Hannes Reinecke wrote:


So if the above reasoning is okay then this patch should be doing
the trick:

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 771c16b..0e72374 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -189,6 +189,7 @@ scsi_abort_command(struct scsi_cmnd *scmd)
 /*
  * Retry after abort failed, escalate to next level.
  */
+   scmd->eh_eflags &= ~SCSI_EH_ABORT_SCHEDULED;
 SCSI_LOG_ERROR_RECOVERY(3,
 scmd_printk(KERN_INFO, scmd,
 "scmd %p previous abort
failed\n", scmd));

(Beware of line
breaks)

Can you test with it?


I don't understand.  This doesn't solve the fundamental problem (namely
that you escalate before aborting a running command).  All it does is
clear the SCSI_EH_ABORT_SCHEDULED flag before escalating.


Which was precisely the point :-)

Hmm. The comment might've been clearer.

What this patch is _supposed_ to be doing is that it'll clear the
SCSI_EH_ABORT_SCHEDULED flag it it has been set.
Which means this will be the second time scsi_abort_command() has
been called for the same command.
IE the first abort went out, did its thing, but now the same command
has timed out again.

So this flag gets cleared, and scsi_abort_command() returns FAILED,
and _no_ asynchronous abort is being scheduled.
scsi_times_out() will then proceed to call scsi_eh_scmd_add().
But as we've cleared the SCSI_EH_ABORT_SCHEDULED flag
the SCSI_EH_CANCEL_CMD flag will continue to be set,
and the command will be aborted with the main SCSI EH routine.

It looks to me as if it should do what you desire, namely abort the
command asynchronously the first time, and invoking the SCSI EH the
second time.

Am I wrong?


I don't know -- I'll have to try it out.  Currently I'm busy with a
bunch of other stuff, so it will take some time.


I finally got a chance to try it out.  It does seem to do what we want.
I didn't track the flow of control in complete detail, but the command
definitely got aborted both times it was issued.

Good, so it is as I thought. James, can we include this patch instead of 
your prior solution?


Cheers,

Hannes
--
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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 0/3] Fix USB deadlock caused by SCSI error handling

2014-04-01 Thread Hannes Reinecke
On 04/01/2014 11:28 PM, Alan Stern wrote:
> On Tue, 1 Apr 2014, Hannes Reinecke wrote:
> 
>>>> So if the above reasoning is okay then this patch should be doing
>>>> the trick:
>>>>
>>>> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
>>>> index 771c16b..0e72374 100644
>>>> --- a/drivers/scsi/scsi_error.c
>>>> +++ b/drivers/scsi/scsi_error.c
>>>> @@ -189,6 +189,7 @@ scsi_abort_command(struct scsi_cmnd *scmd)
>>>> /*
>>>>  * Retry after abort failed, escalate to next level.
>>>>  */
>>>> +   scmd->eh_eflags &= ~SCSI_EH_ABORT_SCHEDULED;
>>>> SCSI_LOG_ERROR_RECOVERY(3,
>>>> scmd_printk(KERN_INFO, scmd,
>>>> "scmd %p previous abort
>>>> failed\n", scmd));
>>>>
>>>> (Beware of line
>>>> breaks)
>>>>
>>>> Can you test with it?
>>>
>>> I don't understand.  This doesn't solve the fundamental problem (namely 
>>> that you escalate before aborting a running command).  All it does is 
>>> clear the SCSI_EH_ABORT_SCHEDULED flag before escalating.
>>>
>> Which was precisely the point :-)
>>
>> Hmm. The comment might've been clearer.
>>
>> What this patch is _supposed_ to be doing is that it'll clear the
>> SCSI_EH_ABORT_SCHEDULED flag it it has been set.
>> Which means this will be the second time scsi_abort_command() has
>> been called for the same command.
>> IE the first abort went out, did its thing, but now the same command
>> has timed out again.
>>
>> So this flag gets cleared, and scsi_abort_command() returns FAILED,
>> and _no_ asynchronous abort is being scheduled.
>> scsi_times_out() will then proceed to call scsi_eh_scmd_add().
>> But as we've cleared the SCSI_EH_ABORT_SCHEDULED flag
>> the SCSI_EH_CANCEL_CMD flag will continue to be set,
>> and the command will be aborted with the main SCSI EH routine.
>>
>> It looks to me as if it should do what you desire, namely abort the
>> command asynchronously the first time, and invoking the SCSI EH the
>> second time.
>>
>> Am I wrong?
> 
> I don't know -- I'll have to try it out.  Currently I'm busy with a 
> bunch of other stuff, so it will take some time.
> 
> Looking through the code, I have to wonder why scsi_times_out()  
> modifies scmd->result.  Won't this value get overwritten by the LLDD
> when the command eventually terminates?
> 
Yes. However, the 'DID_TIME_OUT' is just a marker that the internal
timeout code triggered.
If the LLDD overwrites it with a 'real' error code everything's fine.

> Even worse, what happens in the event of a race where the command 
> terminates normally just before scsi_times_out() changes scmd->result?
> 
_That_ is the least of our worries.
_If_ the LLDD completes the command while scsi_times_out() is
running the whole thing is going down the drain anyway, as the
command will be terminated by the LLDD, and we can only hope that we
didn't mess up our reference counting. Otherwise we'd have EH
running on a stale command, which is going to be a fun to watch.

But looking closer, it might be that the line modifying the result
in scsi_times_out() is indeed pointless, seeing that it's being set
in scsi_eh_abort_handler(), too.
I'll be checking if we can simply remove that line.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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 0/3] Fix USB deadlock caused by SCSI error handling

2014-04-01 Thread Hannes Reinecke
On 04/01/2014 12:29 AM, Alan Stern wrote:
> On Mon, 31 Mar 2014, Hannes Reinecke wrote:
> 
>>>> Ah. Correct. But that's due to the first patch being incorrect.
>>>> Cf my response to the original first patch.
>>>
>>> See my response to your response.  :-)
>>>
>> Okay, So I probably should refrain from issueing a response to
>> your response to my response lest infinite recursion happens :-)
> 
> Indeed.
> 
>>>>>   What should happen if some other pathway manages to call
>>>>>   scsi_try_to_abort_cmd() while scmd->abort_work is still
>>>>>   sitting on the work queue?  The command would be aborted
>>>>>   and the flag would be cleared, but the queued work would
>>>>>   remain.  Can this ever happen?
>>>>>
>>>> Not that I could see.
>>>> A command abort is only ever triggered by the request timeout from
>>>> the block layer. And the timer is _not_ rearmed once the timeout
>>>> function (here: scsi_times_out()) is called.
>>>> Hence I fail to see how it can be called concurrently.
>>>
>>> scsi_try_to_abort_cmd() is also called (via a different pathway) when a 
>>> command sent by the error handler itself times out.  I haven't traced 
>>> through all the different paths to make sure none of them can run 
>>> concurrently.  But I'm willing to take your word for it.
>>>
>> Yes, but that's not calling scsi_abort_command(), but rather invokes
>> scsi_abort_eh_cmnd().
> 
> Sure.  But either way, we end up in scsi_try_to_abort_cmd(), which
> calls the LLDD's abort handler.  Thus leading to the possibility of
> aborting the same command more than once.
> 
>>>> The original idea was this:
>>>>
>>>> SCSI_EH_ABORT_SCHEDULED is sticky _per command_.
>>>> Point is, any command abort is only ever send for a timed-out
>>>> command. And the main problem for a timed-out command is that we
>>>> simply _do not_ know what happened for that command. So _if_ a
>>>> command timed out, _and_ we've send an abort, _and_ the command
>>>> times out _again_ we'll be running into an endless loop between
>>>> timeout and aborting, and never returning the command at all.
>>>>
>>>> So to prevent this we should set a marker on that command telling it
>>>> to _not_ try to abort the command again.
>>>
>>> I disagree.  We _have_ to abort the command again -- how else can we
>>> stop a running command?  To prevent the loop you described, we should
>>> avoid _retrying_ the command after it is aborted the second time.
>>>
>> The actual question is whether it's worth aborting the same command
>> a second time.
>> In principle any reset (like LUN reset etc) should clear the
>> command, too.
>> And the EH abort functionality is geared around this.
>> If, for some reason, the transport layer / device driver
>> requires a command abort to be send then sure, we need
>> to accommodate for that.
> 
> As James mentioned, with USB a reset does not abort a running command.  
> Instead it waits for the command to finish.  (However, this could be
> changed in usb-storage, if required.)
> 
>> As said, yes, in principle you are right. We should be aborting the
>> command a second time, _and then_ starting the escalation.
>>
>> So if the above reasoning is okay then this patch should be doing
>> the trick:
>>
>> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
>> index 771c16b..0e72374 100644
>> --- a/drivers/scsi/scsi_error.c
>> +++ b/drivers/scsi/scsi_error.c
>> @@ -189,6 +189,7 @@ scsi_abort_command(struct scsi_cmnd *scmd)
>> /*
>>  * Retry after abort failed, escalate to next level.
>>  */
>> +   scmd->eh_eflags &= ~SCSI_EH_ABORT_SCHEDULED;
>> SCSI_LOG_ERROR_RECOVERY(3,
>> scmd_printk(KERN_INFO, scmd,
>> "scmd %p previous abort
>> failed\n", scmd));
>>
>> (Beware of line
>> breaks)
>>
>> Can you test with it?
> 
> I don't understand.  This doesn't solve the fundamental problem (namely 
> that you escalate before aborting a running command).  All it does is 
> clear the SCSI_EH_ABORT_SCHEDULED flag before escalating.
> 
Which was precisely the point :-)

Hmm. The comment might've been clearer.

What this patch is _supposed_ 

Re: [PATCH 0/3] Fix USB deadlock caused by SCSI error handling

2014-03-31 Thread Hannes Reinecke
On 03/31/2014 05:03 PM, James Bottomley wrote:
> [lets split the thread]
> On Mon, 2014-03-31 at 16:37 +0200, Hannes Reinecke wrote:
>> On 03/31/2014 03:33 PM, Alan Stern wrote:
>>> On Mon, 31 Mar 2014, Hannes Reinecke wrote:
>>>> On 03/28/2014 08:29 PM, Alan Stern wrote:
>>>>> On Fri, 28 Mar 2014, James Bottomley wrote:
>>>>> Maybe scmd_eh_abort_handler() should check the flag before doing
>>>>> anything.  Is there any sort of sychronization to prevent the same
>>>>> incarnation of a command from being aborted twice (or by two different
>>>>> threads at the same time)?  If there is, it isn't obvious.
>>>>>
>>>> See above. scsi_times_out() will only ever called once.
>>>> What can happen, though, is that _theoretically_ the LLDD might
>>>> decide to call ->done() on a timed out command when
>>>> scsi_eh_abort_handler() is still pending.
>>>
>>> That's okay.  We can expect the LLDD to have sufficient locking to
>>> handle that sort of thing without confusion (usb-storage does, for
>>> example).
>>>
>>>>> (Also, what's going on at the start of scsi_abort_command()?  Contrary
>>>>> to what one might expect, the first part of the function _cancels_ a
>>>>> scheduled abort.  And it does so without clearing the
>>>>> SCSI_EH_ABORT_SCHEDULED flag.)
>>>>>
>>>> The original idea was this:
>>>>
>>>> SCSI_EH_ABORT_SCHEDULED is sticky _per command_.
>>>> Point is, any command abort is only ever send for a timed-out
>>>> command. And the main problem for a timed-out command is that we
>>>> simply _do not_ know what happened for that command. So _if_ a
>>>> command timed out, _and_ we've send an abort, _and_ the command
>>>> times out _again_ we'll be running into an endless loop between
>>>> timeout and aborting, and never returning the command at all.
>>>>
>>>> So to prevent this we should set a marker on that command telling it
>>>> to _not_ try to abort the command again.
>>>
>>> I disagree.  We _have_ to abort the command again -- how else can we
>>> stop a running command?  To prevent the loop you described, we should
>>> avoid _retrying_ the command after it is aborted the second time.
>>>
>> The actual question is whether it's worth aborting the same command
>> a second time.
>> In principle any reset (like LUN reset etc) should clear the
>> command, too.
>> And the EH abort functionality is geared around this.
>> If, for some reason, the transport layer / device driver
>> requires a command abort to be send then sure, we need
>> to accommodate for that.
> 
> We already discussed this (that was my first response too).  USB needs
> all outstanding commands aborted before proceeding to reset.  I'm
> starting to think the actual way to fix this is to reset the abort
> scheduled only if we send something else, so this might be a better fix.
> 
> It doesn't matter if we finish a command with abort scheduled because
> the command then gets freed and the flags cleaned.
> 
> We can take our time with this because the other two patches, which I
> can send separately fix the current deadlock (we no longer send an
> unaborted request sense before the reset), and it can go via rc fixes.
> 
Yes, agreed.

The USB case seems to be a bit more tricky, and at least I need some
more time to fully understand the details and implications of this.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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 0/3] Fix USB deadlock caused by SCSI error handling

2014-03-31 Thread Hannes Reinecke
On 03/31/2014 03:33 PM, Alan Stern wrote:
> On Mon, 31 Mar 2014, Hannes Reinecke wrote:
> 
>> On 03/28/2014 08:29 PM, Alan Stern wrote:
>>> On Fri, 28 Mar 2014, James Bottomley wrote:
>>>
>>>> This is a set of three patches we agreed to a while ago to eliminate a
>>>> USB deadlock.  I did rewrite the first patch, if it could be reviewed
>>>> and tested.
>>>
>>> I tested all three patches under the same conditions as before, and 
>>> they all worked correctly.
>>>
>>> In the revised patch 1, the meaning of SCSI_EH_ABORT_SCHEDULED isn't
>>> entirely clear.  This boils down to two questions, which I don't 
>>> know the answers to:
>>>
>>> What should happen in scmd_eh_abort_handler() if
>>> scsi_host_eh_past_deadline() returns true and thereby
>>> prevents scsi_try_to_abort_cmd() from being called?
>>> The flag wouldn't get cleared then.
>>>
>> Ah. Correct. But that's due to the first patch being incorrect.
>> Cf my response to the original first patch.
> 
> See my response to your response.  :-)
> 
Okay, So I probably should refrain from issueing a response to
your response to my response lest infinite recursion happens :-)

>>> What should happen if some other pathway manages to call
>>> scsi_try_to_abort_cmd() while scmd->abort_work is still
>>> sitting on the work queue?  The command would be aborted
>>> and the flag would be cleared, but the queued work would
>>> remain.  Can this ever happen?
>>>
>> Not that I could see.
>> A command abort is only ever triggered by the request timeout from
>> the block layer. And the timer is _not_ rearmed once the timeout
>> function (here: scsi_times_out()) is called.
>> Hence I fail to see how it can be called concurrently.
> 
> scsi_try_to_abort_cmd() is also called (via a different pathway) when a 
> command sent by the error handler itself times out.  I haven't traced 
> through all the different paths to make sure none of them can run 
> concurrently.  But I'm willing to take your word for it.
> 
Yes, but that's not calling scsi_abort_command(), but rather invokes
scsi_abort_eh_cmnd().

>>> Maybe scmd_eh_abort_handler() should check the flag before doing
>>> anything.  Is there any sort of sychronization to prevent the same
>>> incarnation of a command from being aborted twice (or by two different
>>> threads at the same time)?  If there is, it isn't obvious.
>>>
>> See above. scsi_times_out() will only ever called once.
>> What can happen, though, is that _theoretically_ the LLDD might
>> decide to call ->done() on a timed out command when
>> scsi_eh_abort_handler() is still pending.
> 
> That's okay.  We can expect the LLDD to have sufficient locking to
> handle that sort of thing without confusion (usb-storage does, for
> example).
> 
>>> (Also, what's going on at the start of scsi_abort_command()?  Contrary
>>> to what one might expect, the first part of the function _cancels_ a
>>> scheduled abort.  And it does so without clearing the
>>> SCSI_EH_ABORT_SCHEDULED flag.)
>>>
>> The original idea was this:
>>
>> SCSI_EH_ABORT_SCHEDULED is sticky _per command_.
>> Point is, any command abort is only ever send for a timed-out
>> command. And the main problem for a timed-out command is that we
>> simply _do not_ know what happened for that command. So _if_ a
>> command timed out, _and_ we've send an abort, _and_ the command
>> times out _again_ we'll be running into an endless loop between
>> timeout and aborting, and never returning the command at all.
>>
>> So to prevent this we should set a marker on that command telling it
>> to _not_ try to abort the command again.
> 
> I disagree.  We _have_ to abort the command again -- how else can we
> stop a running command?  To prevent the loop you described, we should
> avoid _retrying_ the command after it is aborted the second time.
> 
The actual question is whether it's worth aborting the same command
a second time.
In principle any reset (like LUN reset etc) should clear the
command, too.
And the EH abort functionality is geared around this.
If, for some reason, the transport layer / device driver
requires a command abort to be send then sure, we need
to accommodate for that.

>> Which is what 'SCSI_EH_ABORT_SCHEDULED' was meant for:
>>
>> - A command times out, sets 'SCSI_EH_ABORT_SCHEDULED'
>> - abort _succeeds_
>> - The same command times out a

Re: [PATCH 0/3] Fix USB deadlock caused by SCSI error handling

2014-03-31 Thread Hannes Reinecke
On 03/28/2014 08:29 PM, Alan Stern wrote:
> On Fri, 28 Mar 2014, James Bottomley wrote:
> 
>> This is a set of three patches we agreed to a while ago to eliminate a
>> USB deadlock.  I did rewrite the first patch, if it could be reviewed
>> and tested.
> 
> I tested all three patches under the same conditions as before, and 
> they all worked correctly.
> 
> In the revised patch 1, the meaning of SCSI_EH_ABORT_SCHEDULED isn't
> entirely clear.  This boils down to two questions, which I don't 
> know the answers to:
> 
>   What should happen in scmd_eh_abort_handler() if
>   scsi_host_eh_past_deadline() returns true and thereby
>   prevents scsi_try_to_abort_cmd() from being called?
>   The flag wouldn't get cleared then.
> 
Ah. Correct. But that's due to the first patch being incorrect.
Cf my response to the original first patch.

>   What should happen if some other pathway manages to call
>   scsi_try_to_abort_cmd() while scmd->abort_work is still
>   sitting on the work queue?  The command would be aborted
>   and the flag would be cleared, but the queued work would
>   remain.  Can this ever happen?
> 
Not that I could see.
A command abort is only ever triggered by the request timeout from
the block layer. And the timer is _not_ rearmed once the timeout
function (here: scsi_times_out()) is called.
Hence I fail to see how it can be called concurrently.

> Maybe scmd_eh_abort_handler() should check the flag before doing
> anything.  Is there any sort of sychronization to prevent the same
> incarnation of a command from being aborted twice (or by two different
> threads at the same time)?  If there is, it isn't obvious.
> 
See above. scsi_times_out() will only ever called once.
What can happen, though, is that _theoretically_ the LLDD might
decide to call ->done() on a timed out command when
scsi_eh_abort_handler() is still pending.


> (Also, what's going on at the start of scsi_abort_command()?  Contrary
> to what one might expect, the first part of the function _cancels_ a
> scheduled abort.  And it does so without clearing the
> SCSI_EH_ABORT_SCHEDULED flag.)
> 
The original idea was this:

SCSI_EH_ABORT_SCHEDULED is sticky _per command_.
Point is, any command abort is only ever send for a timed-out
command. And the main problem for a timed-out command is that we
simply _do not_ know what happened for that command. So _if_ a
command timed out, _and_ we've send an abort, _and_ the command
times out _again_ we'll be running into an endless loop between
timeout and aborting, and never returning the command at all.

So to prevent this we should set a marker on that command telling it
to _not_ try to abort the command again.
Which is what 'SCSI_EH_ABORT_SCHEDULED' was meant for:

- A command times out, sets 'SCSI_EH_ABORT_SCHEDULED'
- abort _succeeds_
- The same command times out a second time, notifies
  that SCSI_EH_ABORT_SCHEDULED is set, and doesn't call
  scsi_eh_abort_command() but rather escalates directly.

_However_, I do feel that we've gotten the wrong track with all of
this. In you original mail you stated:

> Basically, usb-storage deadlocks when the SCSI error handler
> invokes the eh_device_reset_handler callback while a command
> is running.  The command has timed out and will never complete
> normally, because the device's firmware has crashed.  But
> usb-storage's device-reset routine waits for the current command
> to finish, which brings everything to a standstill.

Question now to you as the USB god:

A command abort is only _ever_ send after a command timeout.
And the main idea of the command abort is to remove any context
the LLDD or the target might have. So by the time the command
abort returns SUCCESS we _expect_ the firmware to have cleared that
command. If for some reason the firmware isn't capable of doing so,
it should return FAILED.

So:
- Has the command timeout fired?
- If so, why hasn't it returned FAILED?
- If it had returned SUCCESS, why did the device_reset_handler
  wait for the command to complete?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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 3/3] [SCSI] Fix command result state propagation

2014-03-31 Thread Hannes Reinecke
On 03/28/2014 06:51 PM, James Bottomley wrote:
> From: Alan Stern 
> 
> We're seeing a case where the contents of scmd->result isn't being reset after
> a SCSI command encounters an error, is resubmitted, times out and then gets
> handled.  The error handler acts on the stale result of the previous error
> instead of the timeout.  Fix this by properly zeroing the scmd->status before
> the command is resubmitted.
> 
> Signed-off-by: Alan Stern 
> Signed-off-by: James Bottomley 



Acked-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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 2/3] [SCSI] Fix spurious request sense in error handling

2014-03-31 Thread Hannes Reinecke
On 03/28/2014 06:50 PM, James Bottomley wrote:
> We unconditionally execute scsi_eh_get_sense() to make sure all failed
> commands that should have sense attached, do.  However, the routine forgets
> that some commands, because of the way they fail, will not have any sense code
> ... we should not bother them with a REQUEST_SENSE command.  Fix this by
> testing to see if we actually got a CHECK_CONDITION return and skip asking for
> sense if we don't.
> 
> Tested-by: Alan Stern 
> Signed-off-by: James Bottomley 
Acked-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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 1/3] [SCSI] Fix abort state memory problem

2014-03-30 Thread Hannes Reinecke
On 03/28/2014 06:49 PM, James Bottomley wrote:
> The abort state is being stored persistently across commands, meaning if the
> command times out a second time, the error handler thinks an abort is still
> pending.  Fix this by properly resetting the abort flag after the abort is
> finished.
> 
> Signed-off-by: James Bottomley 
> ---
>  drivers/scsi/scsi_error.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 771c16b..b9f3b07 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -869,10 +869,13 @@ static int scsi_try_bus_device_reset(struct scsi_cmnd 
> *scmd)
>  
>  static int scsi_try_to_abort_cmd(struct scsi_host_template *hostt, struct 
> scsi_cmnd *scmd)
>  {
> - if (!hostt->eh_abort_handler)
> - return FAILED;
> + int retval = FAILED;
> +
> + if (hostt->eh_abort_handler)
> + retval = hostt->eh_abort_handler(scmd);
>  
> - return hostt->eh_abort_handler(scmd);
> + scmd->eh_eflags &= ~SCSI_EH_ABORT_SCHEDULED;
> + return retval;
>  }
>  
>  static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
> 
Hmm. No, I don't think this is correct.

SCSI_EH_ABORT_SCHEDULED signifies whether scmd_eh_abort_handler()
needs to run. As such it needs to be reset when the workqueue item
is triggered.

Can you try with this instead?

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 771c16b..9694803 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -120,6 +120,8 @@ scmd_eh_abort_handler(struct work_struct *work)
struct scsi_device *sdev = scmd->device;
int rtn;

+   scmd->eh_eflags &= ~SCSI_EH_ABORT_SCHEDULED;
+
if (scsi_host_eh_past_deadline(sdev->host)) {
SCSI_LOG_ERROR_RECOVERY(3,
scmd_printk(KERN_INFO, scmd,


Cheers,

Hannes
-- 
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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: XHCI issues: WD MyBook 1230 - reset SuperSpeed USB device

2014-01-16 Thread Hannes Reinecke
On 01/16/2014 09:48 PM, Alan Stern wrote:
> It's now clear that this is _not_ an XHCI issue, contrary to what 
> $SUBJECT says.
> 
> On Thu, 16 Jan 2014, Peter Palúch wrote:
> 
>> Alan,
>>
>> I am attaching the usbmon trace after the drive has been plugged into 
>> the USB-2 port. Record of the drive in stall should occur around the 
>> file offset 87808 (decimal). The log was done on the 3.12.7 kernel 
>> without CONFIG_PM. Should I do a usbmon trace on my regular kernel with 
>> CONFIG_PM as well?
> 
> No need.
> 
>> dmesg transcript:
>>
>> root@bach:/tmp# dmesg
>> usb 4-1.2: new high-speed USB device number 5 using ehci-pci
>> usb 4-1.2: New USB device found, idVendor=1058, idProduct=1230
>> usb 4-1.2: New USB device strings: Mfr=2, Product=3, SerialNumber=1
>> usb 4-1.2: Product: My Book 1230
>> usb 4-1.2: Manufacturer: Western Digital
>> usb 4-1.2: SerialNumber: 574D43344E30323438393836
>> usb-storage 4-1.2:1.0: USB Mass Storage device detected
>> scsi6 : usb-storage 4-1.2:1.0
>> usbcore: registered new interface driver usb-storage
>> scsi 6:0:0:0: Direct-Access WD   My Book 1230 1050 PQ: 0 ANSI: 6
>> scsi 6:0:0:1: Enclosure WD   SES Device   1050 PQ: 0 ANSI: 6
>> sd 6:0:0:0: Attached scsi generic sg2 type 0
>> scsi 6:0:0:1: Attached scsi generic sg3 type 13
>> sd 6:0:0:0: [sdb] Spinning up disk...
>> .ready
>> sd 6:0:0:0: [sdb] 732558336 4096-byte logical blocks: (3.00 TB/2.72 TiB)
>> sd 6:0:0:0: [sdb] Write Protect is off
>> sd 6:0:0:0: [sdb] Mode Sense: 53 00 10 08
>> sd 6:0:0:0: [sdb] No Caching mode page found
>> sd 6:0:0:0: [sdb] Assuming drive cache: write through
>> sd 6:0:0:0: [sdb] 732558336 4096-byte logical blocks: (3.00 TB/2.72 TiB)
>> sd 6:0:0:0: [sdb] No Caching mode page found
>> sd 6:0:0:0: [sdb] Assuming drive cache: write through
>>   sdb: sdb1
>> sd 6:0:0:0: [sdb] 732558336 4096-byte logical blocks: (3.00 TB/2.72 TiB)
>> sd 6:0:0:0: [sdb] No Caching mode page found
>> sd 6:0:0:0: [sdb] Assuming drive cache: write through
>> sd 6:0:0:0: [sdb] Attached SCSI disk
>> usb 4-1.2: reset high-speed USB device number 5 using ehci-pci
> 
> It looks like the reset occurred because the computer sent an
> ATA-passthru command to the disk, and the disk wasn't prepared to
> handle it properly.  The firmware crashed, requiring a reset.
> 
> If anyone can explain, the command bytes in question were:
> 
>   85082e00   ec00
> 
> and the sense data was:
> 
>   7201001d 000e 090c 5d00 0100 0050
> 
> I don't know what either of these means, or even what software was
> responsible for sending this command.  It appears to have come from
> some user program, though, not the kernel.  Possibly something run by 
> udev.
> 
Probably smartd.
The logic there is _less_ than perfect.
Try to disable smartd and check if the issue remains.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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: dealing with enclosures that don't handle READ_CAPACITY10

2013-08-19 Thread Hannes Reinecke
On 08/12/2013 03:17 PM, Oliver Neukum wrote:
> Hi,
> 
> I got a bug report about an enclosure that mishandles READ_CAPACITY10.
> I don't want to introduce yet another quirk. So what about basing this
> on USB version?
> 
Hmm. You _sure_ it's restricted to USB 3.0? Seem like a generic USB
firmware issue to me, so I'd rather have it handled via a quirk.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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][v2] xhci: correctly enable interrupts

2013-03-04 Thread Hannes Reinecke
xhci has its own interrupt enabling routine, which will try to
use MSI-X/MSI if present. So the usb core shouldn't try to enable
legacy interrupts; on some machines the xhci legacy IRQ setting
is invalid.

Cc: Bjorn Helgaas 
Cc: Oliver Neukum 
Cc: Thomas Renninger 
Cc: Yinghai Lu 
Cc: Frederik Himpe 
Cc: David Haerdeman 
Cc: Alan Stern 
Signed-off-by: Hannes Reinecke 

diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index 622b4a4..2647e75 100644
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -173,6 +173,7 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct 
pci_device_id *id)
struct hc_driver*driver;
struct usb_hcd  *hcd;
int retval;
+   int hcd_irq = 0;
 
if (usb_disabled())
return -ENODEV;
@@ -187,15 +188,21 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct 
pci_device_id *id)
return -ENODEV;
dev->current_state = PCI_D0;
 
-   /* The xHCI driver supports MSI and MSI-X,
-* so don't fail if the BIOS doesn't provide a legacy IRQ.
+   /*
+* The xHCI driver supports MSI and MSI-X,
+* so don't fail if the BIOS doesn't provide a legacy IRQ
+* and do not try to enable legacy IRQs.
 */
-   if (!dev->irq && (driver->flags & HCD_MASK) != HCD_USB3) {
-   dev_err(&dev->dev,
-   "Found HC with no IRQ.  Check BIOS/PCI %s setup!\n",
-   pci_name(dev));
-   retval = -ENODEV;
-   goto disable_pci;
+   if ((driver->flags & HCD_MASK) != HCD_USB3) {
+   if (!dev->irq) {
+   dev_err(&dev->dev,
+   "Found HC with no IRQ.  "
+   "Check BIOS/PCI %s setup!\n",
+   pci_name(dev));
+   retval = -ENODEV;
+   goto disable_pci;
+   }
+   hcd_irq = dev->irq;
}
 
hcd = usb_create_hcd(driver, &dev->dev, pci_name(dev));
@@ -245,7 +252,7 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct 
pci_device_id *id)
 
pci_set_master(dev);
 
-   retval = usb_add_hcd(hcd, dev->irq, IRQF_SHARED);
+   retval = usb_add_hcd(hcd, hcd_irq, IRQF_SHARED);
if (retval != 0)
goto unmap_registers;
set_hs_companion(dev, hcd);
--
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] usb: correctly enable interrupts for xhci

2013-03-01 Thread Hannes Reinecke
xhci might run with MSI/MSI-X only, with no support for legacy
interrupts. On these devices the request_irq() call in usb_add_hcd()
will fail, causing the entire device to fail.
For xhci this is especially painful as the driver will enable
interrupts during xhci_run(), so the initial call to request_irq()
is not required anyway.

This patch adds a flag 'msix_supported' to struct usb_hcd, which
will cause usb_add_hcd() to skip interrupt setup. This flag is
set in xhci-pci, so the registration will skip the first request_irq()
and can proceed.

Cc: Bjorn Helgaas 
Cc: Oliver Neukum 
Cc: Thomas Renninger 
Cc: Yinghai Lu 
Cc: Frederik Himpe 
Cc: David Haerdeman 
Signed-off-by: Hannes Reinecke 

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 8e64adf..e583444 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2536,7 +2536,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
/* enable irqs just before we start the controller,
 * if the BIOS provides legacy PCI irqs.
 */
-   if (usb_hcd_is_primary_hcd(hcd) && irqnum) {
+   if (usb_hcd_is_primary_hcd(hcd) && irqnum && !hcd->msix_supported) {
retval = usb_hcd_request_irqs(hcd, irqnum, irqflags);
if (retval)
goto err_request_irq;
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index af259e0..a08c0ee 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -183,6 +183,12 @@ static int xhci_pci_probe(struct pci_dev *dev, const 
struct pci_device_id *id)
 */
*((struct xhci_hcd **) xhci->shared_hcd->hcd_priv) = xhci;
 
+   /*
+* Interrupts are setup during xhci_run(), so do not try
+* to request an interrupt during hcd init.
+*/
+   hcd->msix_supported = 1;
+
retval = usb_add_hcd(xhci->shared_hcd, dev->irq,
IRQF_SHARED);
if (retval)
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 0a78df5..eb6bf10 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -124,6 +124,7 @@ struct usb_hcd {
/* Flags that get set only during HCD registration or removal. */
unsignedrh_registered:1;/* is root hub registered? */
unsignedrh_pollable:1;  /* may we poll the root hub? */
+   unsignedmsix_supported:1; /* driver supports MSI-X */
unsignedmsix_enabled:1; /* driver has MSI-X enabled? */
 
/* The next flag is a stopgap, to be removed when all the HCDs
--
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] pci: do not try to assign irq 255

2013-02-28 Thread Hannes Reinecke

On 02/27/2013 10:13 PM, Bjorn Helgaas wrote:

[+cc Andy]

On Wed, Feb 20, 2013 at 11:53 PM, Hannes Reinecke  wrote:

On 02/20/2013 05:57 PM, Yinghai Lu wrote:


On Tue, Feb 19, 2013 at 11:58 PM, Hannes Reinecke  wrote:




Apparently this device is meant to use MSI _only_ so the BIOS developer
didn't feel the need to assign an INTx here.

According to PCI-3.0, section 6.8 (Message Signalled Interrupts):


It is recommended that devices implement interrupt pins to
provide compatibility in systems that do not support MSI
(devices default to interrupt pins). However, it is expected
that the need for interrupt pins will diminish over time.
Devices that do not support interrupt pins due to pin
constraints (rely on polling for device service) may implement
messages to increase performance without adding additional pins. >
Therefore, system configuration software must not assume that a
message capable device has an interrupt pin.



Which sounds to me as if the implementation is valid...



it seems you mess pin with interrupt line.

current code:
  unsigned char irq;

  pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &irq);
  dev->pin = irq;
  if (irq)
  pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
  dev->irq = irq;

so if the device does not have interrupt pin implemented, pin should be
zero.
and  pin and irq in dev should
be all 0.


But the device _has_ an interrupt pin implemented.
The whole point here is that the interrupt line is _NOT_ zero.

00:14.0 USB controller [0c03]: Intel Corporation 7 Series/C210 Series
Chipset Family USB xHCI Host Controller [8086:1e31] (rev 04) (prog-if 30
[XHCI])
 Subsystem: Hewlett-Packard Company Device [103c:179b]
 Control: I/O- Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr-
Stepping- SERR- FastB2B- DisINTx-
 Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort-
SERR- irq is not valid, despite it being
not set to zero.
An alternative fix would be this:

diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
index 68a921d..4a480cb 100644
--- a/drivers/acpi/pci_irq.c
+++ b/drivers/acpi/pci_irq.c
@@ -469,6 +469,7 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
 } else {
 dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
  pin_name(pin));
+   dev->irq = 0;
 }
 return 0;
 }

Which probably is a better solution, as here ->irq is _definitely_
not valid, so we should reset it to '0' to avoid confusion on upper
layers.


I didn't like the pci_read_irq() change because the PCI spec doesn't
say anything about any PCI_INTERRUPT_LINE values being invalid.

I like this solution better, but I still don't quite understand it.
We have the following code in acpi_pci_irq_enable().  We have
previously tried to look up "gsi," but the _PRT doesn't mention this
device, so we have "gsi == -1" at this point:

 /*
  * No IRQ known to the ACPI subsystem - maybe the BIOS /
  * driver reported one, then use it. Exit in any case.
  */
 if (gsi < 0) {
 u32 dev_gsi;
 /* Interrupt Line values above 0xF are forbidden */
 if (dev->irq > 0 && (dev->irq <= 0xF) &&
 (acpi_isa_irq_to_gsi(dev->irq, &dev_gsi) == 0)) {
 dev_warn(&dev->dev, "PCI INT %c: no GSI -
using ISA IRQ %d\n",
  pin_name(pin), dev->irq);
 acpi_register_gsi(&dev->dev, dev_gsi,
   ACPI_LEVEL_SENSITIVE,
   ACPI_ACTIVE_LOW);
 } else {
 dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
  pin_name(pin));
 }

 return 0;
 }

1) I don't know where the restriction of 0x1-0xF came from.
Presumably this value of dev->irq came from PCI_INTERRUPT_LINE, and I
don't know what forbids values > 0xF.  The test was added by Andy
Grover in the attached commit.  This is ancient history; probably Andy
doesn't remember either :)


This is most likely due to ISA compability. Cf ACPI 4.0,
section 5.2.12.4 Platforms with APIC and Dual 8259 Support:

> Systems that support both APIC and dual 8259 interrupt models
> must map global system interrupts 0-15 to the 8259 IRQs 0-15,
> except where Interrupt Source Overrides are provided (see section
> 5.2.10.8, “Interrupt Source Overrides”). This means that I/O APIC
> interrupt inputs 0-15 must be mapped to global system interrupts
> 0-15 and have identical sources as the 8259 IRQs 0-15 unless
> overrides are u

Re: [PATCH] pci: do not try to assign irq 255

2013-02-28 Thread Hannes Reinecke

On 02/27/2013 10:13 PM, Bjorn Helgaas wrote:

[+cc Andy]

On Wed, Feb 20, 2013 at 11:53 PM, Hannes Reinecke  wrote:

On 02/20/2013 05:57 PM, Yinghai Lu wrote:


On Tue, Feb 19, 2013 at 11:58 PM, Hannes Reinecke  wrote:




Apparently this device is meant to use MSI _only_ so the BIOS developer
didn't feel the need to assign an INTx here.

According to PCI-3.0, section 6.8 (Message Signalled Interrupts):


It is recommended that devices implement interrupt pins to
provide compatibility in systems that do not support MSI
(devices default to interrupt pins). However, it is expected
that the need for interrupt pins will diminish over time.
Devices that do not support interrupt pins due to pin
constraints (rely on polling for device service) may implement
messages to increase performance without adding additional pins. >
Therefore, system configuration software must not assume that a
message capable device has an interrupt pin.



Which sounds to me as if the implementation is valid...



it seems you mess pin with interrupt line.

current code:
  unsigned char irq;

  pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &irq);
  dev->pin = irq;
  if (irq)
  pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
  dev->irq = irq;

so if the device does not have interrupt pin implemented, pin should be
zero.
and  pin and irq in dev should
be all 0.


But the device _has_ an interrupt pin implemented.
The whole point here is that the interrupt line is _NOT_ zero.

00:14.0 USB controller [0c03]: Intel Corporation 7 Series/C210 Series
Chipset Family USB xHCI Host Controller [8086:1e31] (rev 04) (prog-if 30
[XHCI])
 Subsystem: Hewlett-Packard Company Device [103c:179b]
 Control: I/O- Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr-
Stepping- SERR- FastB2B- DisINTx-
 Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort-
SERR- irq is not valid, despite it being
not set to zero.
An alternative fix would be this:

diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
index 68a921d..4a480cb 100644
--- a/drivers/acpi/pci_irq.c
+++ b/drivers/acpi/pci_irq.c
@@ -469,6 +469,7 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
 } else {
 dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
  pin_name(pin));
+   dev->irq = 0;
 }
 return 0;
 }

Which probably is a better solution, as here ->irq is _definitely_
not valid, so we should reset it to '0' to avoid confusion on upper
layers.


I didn't like the pci_read_irq() change because the PCI spec doesn't
say anything about any PCI_INTERRUPT_LINE values being invalid.

I like this solution better, but I still don't quite understand it.
We have the following code in acpi_pci_irq_enable().  We have
previously tried to look up "gsi," but the _PRT doesn't mention this
device, so we have "gsi == -1" at this point:

 /*
  * No IRQ known to the ACPI subsystem - maybe the BIOS /
  * driver reported one, then use it. Exit in any case.
  */
 if (gsi < 0) {
 u32 dev_gsi;
 /* Interrupt Line values above 0xF are forbidden */
 if (dev->irq > 0 && (dev->irq <= 0xF) &&
 (acpi_isa_irq_to_gsi(dev->irq, &dev_gsi) == 0)) {
 dev_warn(&dev->dev, "PCI INT %c: no GSI -
using ISA IRQ %d\n",
  pin_name(pin), dev->irq);
 acpi_register_gsi(&dev->dev, dev_gsi,
   ACPI_LEVEL_SENSITIVE,
   ACPI_ACTIVE_LOW);
 } else {
 dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
  pin_name(pin));
 }

 return 0;
 }

1) I don't know where the restriction of 0x1-0xF came from.
Presumably this value of dev->irq came from PCI_INTERRUPT_LINE, and I
don't know what forbids values > 0xF.  The test was added by Andy
Grover in the attached commit.  This is ancient history; probably Andy
doesn't remember either :)

2) The proposed change (setting "dev->irq = 0" when we didn't find
anything in the _PRT and dev->irq > 0xF) throws away some information,
and I fear it could break systems.  For example, what would happen if
a system put an IOAPIC pin number in a device's PCI_INTERRUPT_LINE and
omitted the device from _PRT?  Is it possible the device would still
work as-is (with acpi_pci_irq_enable() doing nothing), but would break
if we set dev->irq = 0?

3) I don't understand why the xhci init fails in the first place.  It

Re: [PATCH] pci: do not try to assign irq 255

2013-02-26 Thread Hannes Reinecke

On 02/26/2013 02:29 PM, David Härdeman wrote:

On Thu, Feb 21, 2013 at 07:53:14AM +0100, Hannes Reinecke wrote:

On 02/20/2013 05:57 PM, Yinghai Lu wrote:

it seems you mess pin with interrupt line.

current code:
 unsigned char irq;

 pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &irq);
 dev->pin = irq;
 if (irq)
 pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
 dev->irq = irq;

so if the device does not have interrupt pin implemented, pin should be zero.
and  pin and irq in dev should
be all 0.


But the device _has_ an interrupt pin implemented.
The whole point here is that the interrupt line is _NOT_ zero.


...


So at one point we have to decide that ->irq is not valid, despite it
being not set to zero.
An alternative fix would be this:

diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
index 68a921d..4a480cb 100644
--- a/drivers/acpi/pci_irq.c
+++ b/drivers/acpi/pci_irq.c
@@ -469,6 +469,7 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
} else {
dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
 pin_name(pin));
+   dev->irq = 0;
}
return 0;
}

Which probably is a better solution, as here ->irq is _definitely_
not valid, so we should reset it to '0' to avoid confusion on upper
layers.



Is there any agreement on how to proceed?


I would actually prefer the second solution, as the ACPI code gives
some better guarantees here. With the original solution it _might_ 
be that on non-ACPI systems an interrupt 255 is valid, so it might

incur unwanted regressions.

However, for an ACPI system we only have the two choices, assigning
an interrupt via ACPI tables or use a default GSI value.
If both failed the interrupt definitely is not valid and can safely
be reset to 0.

But this would need a formal ACK from the ACPI gods ...
Len? Rafael?

Cheers,

Hannes
--
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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] pci: do not try to assign irq 255

2013-02-20 Thread Hannes Reinecke

On 02/20/2013 05:57 PM, Yinghai Lu wrote:

On Tue, Feb 19, 2013 at 11:58 PM, Hannes Reinecke  wrote:



Apparently this device is meant to use MSI _only_ so the BIOS developer
didn't feel the need to assign an INTx here.

According to PCI-3.0, section 6.8 (Message Signalled Interrupts):

It is recommended that devices implement interrupt pins to
provide compatibility in systems that do not support MSI
(devices default to interrupt pins). However, it is expected
that the need for interrupt pins will diminish over time.
Devices that do not support interrupt pins due to pin
constraints (rely on polling for device service) may implement
messages to increase performance without adding additional pins. >
Therefore, system configuration software must not assume that a
message capable device has an interrupt pin.


Which sounds to me as if the implementation is valid...


it seems you mess pin with interrupt line.

current code:
 unsigned char irq;

 pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &irq);
 dev->pin = irq;
 if (irq)
 pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
 dev->irq = irq;

so if the device does not have interrupt pin implemented, pin should be zero.
and  pin and irq in dev should
be all 0.


But the device _has_ an interrupt pin implemented.
The whole point here is that the interrupt line is _NOT_ zero.

00:14.0 USB controller [0c03]: Intel Corporation 7 Series/C210 
Series Chipset Family USB xHCI Host Controller [8086:1e31] (rev 04) 
(prog-if 30 [XHCI])

Subsystem: Hewlett-Packard Company Device [103c:179b]
	Control: I/O- Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- 
Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- 
SERR- 
Interrupt: pin A routed to IRQ 255
Region 0: Memory at d472 (64-bit, non-prefetchable) [size=64K]
Capabilities: [70] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=375mA 
PME(D0-,D1-,D2-,D3hot+,D3cold+)

Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
Capabilities: [80] MSI: Enable- Count=1/8 Maskable- 64bit+
Address:   Data: 

So at one point we have to decide that ->irq is not valid, despite 
it being not set to zero.

An alternative fix would be this:

diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
index 68a921d..4a480cb 100644
--- a/drivers/acpi/pci_irq.c
+++ b/drivers/acpi/pci_irq.c
@@ -469,6 +469,7 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
} else {
dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
 pin_name(pin));
+   dev->irq = 0;
}
return 0;
}

Which probably is a better solution, as here ->irq is _definitely_
not valid, so we should reset it to '0' to avoid confusion on upper
layers.

Cheers,

Hannes
--
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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] pci: do not try to assign irq 255

2013-02-19 Thread Hannes Reinecke

On 02/19/2013 08:40 PM, Yinghai Lu wrote:

On Mon, Feb 18, 2013 at 2:09 AM, Hannes Reinecke  wrote:

The PCI config space reseves a byte for the interrupt line,
so irq 255 actually refers to 'not set'.
However, the 'irq' field for struct pci_dev is an integer,
so the original meaning is lost, causing the system to
assign an interrupt '255', which fails.

So we should _not_ assign an interrupt value here, and
allow upper layers to fixup things.

This patch make PCI devices with MSI interrupts only
(like the xhci device on certain HP laptops) work properly.


looks like the bios does not provide _PRT for device in ACPI.


Correct.


also according to PCI spec, BIOS *must* set interrupt line.

Apparently this device is meant to use MSI _only_ so the BIOS 
developer didn't feel the need to assign an INTx here.


According to PCI-3.0, section 6.8 (Message Signalled Interrupts):
> It is recommended that devices implement interrupt pins to
> provide compatibility in systems that do not support MSI
> (devices default to interrupt pins). However, it is expected
> that the need for interrupt pins will diminish over time.
> Devices that do not support interrupt pins due to pin
> constraints (rely on polling for device service) may implement
> messages to increase performance without adding additional pins. 
> Therefore, system configuration software must not assume that a

> message capable device has an interrupt pin.

Which sounds to me as if the implementation is valid...
And in either case, I've added the relevant details plus patch
to bnc#52591.
Including ACPI dump, so you can check for yourself.

And correct me if I'm wrong, of course :-)

Cheers,

Hannes
--
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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] pci: do not try to assign irq 255

2013-02-18 Thread Hannes Reinecke
The PCI config space reseves a byte for the interrupt line,
so irq 255 actually refers to 'not set'.
However, the 'irq' field for struct pci_dev is an integer,
so the original meaning is lost, causing the system to
assign an interrupt '255', which fails.

So we should _not_ assign an interrupt value here, and
allow upper layers to fixup things.

This patch make PCI devices with MSI interrupts only
(like the xhci device on certain HP laptops) work properly.

Cc: Frederik Himpe 
Cc: Oliver Neukum 
Cc: David Haerdeman 
Cc: linux-usb@vger.kernel.org
Cc: linux-...@vger.kernel.org
Signed-off-by: Hannes Reinecke 

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6186f03..a2db887f 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -923,7 +923,8 @@ static void pci_read_irq(struct pci_dev *dev)
dev->pin = irq;
if (irq)
pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
-   dev->irq = irq;
+   if (irq < 255)
+   dev->irq = irq;
 }
 
 void set_pcie_port_type(struct pci_dev *pdev)
--
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