Re: Page allocation failure (order 7) in UAS code

[Hans de Goede]

Hi,

On 04-03-16 08:13, Yves-Alexis Perez wrote:

On mar., 2016-03-01 at 11:49 +0100, Hans de Goede wrote:

Hi,

On 01-03-16 10:42, Yves-Alexis Perez wrote:


Hi,

[sorry if this is not the right point for reporting bugs, I took the email
addresses from MAINTAINERS but please point me to the correct place if
needed]

I have an external USB drive (Samsung M3), which apparently uses the UAS
code.
Starting with 4.4 (from Debian sid, I could retry with vanilla if needed),
I
can't mount the drive anymore after a while (few hours/days uptime). Just
plugging the disk, I get page allocation failure in kernel logs:

Can you try building a kernel with the following line in
drivers/usb/storage/uas.c :

  .can_queue = 65536, /* Is there a limit on the _host_ ? */

(around line 815) Replaced with

  .can_queue = MAX_CMNDS,

That should help as MAX_CMNDS is 256, so claiming that we can queue more
is not helpful, and that likely is what is causing this quite high order
alloc.


After a few days, it seems that it does work fine, although I can't say
anything about sides effects.


Thanks for testing, there shouldn't be any side-effects, I'll turn this into
a proper patch, add a:

Reported-and-tested-by: Yves-Alexis Perez 

line to the comit msg and submit this upstream.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Page allocation failure (order 7) in UAS code

[Yves-Alexis Perez]

On mar., 2016-03-01 at 11:49 +0100, Hans de Goede wrote:
> Hi,
> 
> On 01-03-16 10:42, Yves-Alexis Perez wrote:
> > 
> > Hi,
> > 
> > [sorry if this is not the right point for reporting bugs, I took the email
> > addresses from MAINTAINERS but please point me to the correct place if
> > needed]
> > 
> > I have an external USB drive (Samsung M3), which apparently uses the UAS
> > code.
> > Starting with 4.4 (from Debian sid, I could retry with vanilla if needed),
> > I
> > can't mount the drive anymore after a while (few hours/days uptime). Just
> > plugging the disk, I get page allocation failure in kernel logs:
> Can you try building a kernel with the following line in
> drivers/usb/storage/uas.c :
> 
>  .can_queue = 65536, /* Is there a limit on the _host_ ? */
> 
> (around line 815) Replaced with
> 
>  .can_queue = MAX_CMNDS,
> 
> That should help as MAX_CMNDS is 256, so claiming that we can queue more
> is not helpful, and that likely is what is causing this quite high order
> alloc.

After a few days, it seems that it does work fine, although I can't say
anything about sides effects.

Regards,
-- 
Yves-Alexis



signature.asc
Description: This is a digitally signed message part

Re: [PATCHv2 1/6] scsi: Add 'access_state' and 'preferred_path' attribute

[Bart Van Assche]

On 03/02/2016 10:54 PM, Hannes Reinecke wrote:

Add an 'access_state' field to struct scsi_device
and display them in sysfs as 'access_state' and
'preferred_path' attribute.


Reviewed-by: Bart van Assche 
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 01/15] scsi: ufs-qcom: add number of lanes per direction

[Rob Herring]

On Sun, Feb 28, 2016 at 03:32:33PM +0200, Yaniv Gardi wrote:
> Different platform may have different number of lanes
> for the UFS link.
> Add parameter to device tree specifying how many lanes
> should be configured for the UFS link.
> 
> Signed-off-by: Gilad Broner 
> Signed-off-by: Yaniv Gardi 
> 
> ---
>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt  |  3 ++

Acked-by: Rob Herring 

>  drivers/scsi/ufs/ufs-qcom.c| 39 
> --
>  drivers/scsi/ufs/ufshcd-pltfrm.c   | 19 +++
>  drivers/scsi/ufs/ufshcd.c  |  1 +
>  drivers/scsi/ufs/ufshcd.h  |  2 ++
>  5 files changed, 47 insertions(+), 17 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 3/3] add support for DWC UFS Host Controller

[Joao Pinto]

On 3/3/2016 2:12 PM, Arnd Bergmann wrote:
> On Thursday 03 March 2016 13:52:39 Joao Pinto wrote:
>>
>> config SCSI_UFS_DWC
>> bool
>>
>> config SCSI_UFS_DWC_TC_PLATFORM
>> tristate "DesignWare platform support using a G210 Test Chip"
>> depends on SCSI_UFSHCD_PLATFORM
>> select SCSI_UFS_DWC
>> ---help---
>>   Synopsys Test Chip is a PHY for prototyping purposes.
>>
>>   If unsure, say N."
>>
>> config SCSI_UFS_DWC_TC_PCI
>> tristate "DesignWare pci support using a G210 Test Chip"
>> depends on SCSI_UFSHCD_PCI
>> select SCSI_UFS_DWC
>> ---help---
>>   Synopsys Test Chip is a PHY for prototyping purposes.
>>
>>   If unsure, say N."
>>
>> I would keep SCSI_UFS_DWC to avoid building DWC specific source everytime.
>>
>> Agree?
>>
> 
> Yes, looks good to me.
> 
>   Arnd
> 

Nice! Thanks for the inputs. I'm going to implement the changes and send a v10 
soon.

Joao
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 3/3] add support for DWC UFS Host Controller

[Arnd Bergmann]

On Thursday 03 March 2016 13:52:39 Joao Pinto wrote:
> 
> config SCSI_UFS_DWC
> bool
> 
> config SCSI_UFS_DWC_TC_PLATFORM
> tristate "DesignWare platform support using a G210 Test Chip"
> depends on SCSI_UFSHCD_PLATFORM
> select SCSI_UFS_DWC
> ---help---
>   Synopsys Test Chip is a PHY for prototyping purposes.
> 
>   If unsure, say N."
> 
> config SCSI_UFS_DWC_TC_PCI
> tristate "DesignWare pci support using a G210 Test Chip"
> depends on SCSI_UFSHCD_PCI
> select SCSI_UFS_DWC
> ---help---
>   Synopsys Test Chip is a PHY for prototyping purposes.
> 
>   If unsure, say N."
> 
> I would keep SCSI_UFS_DWC to avoid building DWC specific source everytime.
> 
> Agree?
> 

Yes, looks good to me.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 3/3] add support for DWC UFS Host Controller

[Joao Pinto]

Hi,

On 3/3/2016 12:04 PM, Arnd Bergmann wrote:
> On Thursday 03 March 2016 11:39:05 Joao Pinto wrote:
>> Hi Arnd,
>>
>> On 3/2/2016 7:55 PM, Arnd Bergmann wrote:
>>> On Wednesday 02 March 2016 16:46:47 Joao Pinto wrote:
 On 2/19/2016 3:03 PM, Arnd Bergmann wrote:
> On Thursday 18 February 2016 17:20:27 Joao Pinto wrote:
>>
>> Facts:
>>
>> - Test Chip type are currently not detectable in runtime through the 
>> controller
>> - In the future the Test Chip version will be available in the controller
>> - Test Chip initialization is different for each type
>> - The IP Core version is 1.40a
>> - Test Chip version is 6.00
>> - Teh UFS version is 2.0
> 
> Ok.
> 
>> Suggested driver architecture:
>>
>> Platform setup:
>>  tc-dwc-g210-pltfrm --> tc-dwc-g210 --> ufshcd-dwc-pltfrm --> ufshcd-dwc --> 
>> ufs
>>
>> The test chip platform driver could be called through 2 compatibility 
>> strings.
>> indicating the chip's version and bit type:
>>  "snps, g210-tc-6.00-20bit"
>>  "snps, g210-tc-6.00-40bit"
> 
> Yes, this sounds good. We can probably skip one of the middle layers,
> but basically that is what I was looking for.
> 
>> The device tree node would have additional info compatibility strings as the 
>> DWC
>> IP core version and UFS version:
>>  "snps, dwc-ufshcd-1.40a"
>>  "jedec, ufs-2.0"
>>
>> PCI based setup:
>>  tc-dwc-g210-pci --> tc-dwc-g210 --> ufshcd-dwc-pci --> ufshcd-dwc --> ufs
> 
> The tc-dwc-g210 portion probably shouldn't depend on both
> ufshcd-dwc-pltfrm and ufshcd-dwc-pci here, so how about leaving
> those two out?
> 
> 
> Then it becomes
> 
>tc-dwc-g210-pci ---> tc-dwc-g210 --> ufshcd-dwc --> ufs
> tc-dwc-g210-pltfrm --/

Ok, understood. It becomes simpler without the pltfm and pci "middle layer".

> 
>> The test chip type would be configured by a parameter to be passed in the 
>> kernel
>> boot args: tc_type = 20 (20-bit) or tc_type = 40 (40-bit)
> 
> Right. With module_param() helper, this will be either a boot command
> line option, or a module load option, depending on whether the driver
> is built-on or not.
> 
> modprobe tc-dwc-g210-pci tc_type=20
> 
> command line: tc-dwc-g210-pci.tc_type=20
>  

Right, that was the idea.

>> Having this in mind the KConfig would be:
>>
>> "config SCSI_UFS_DWC_HOOKS
>>  bool
> 
> I think we can now remove the config option for the hooks as well.
> 
>> config SCSI_UFS_DWC_PLAT
>>  tristate "DesignWare UFS controller platform glue driver"
>>  depends on SCSI_UFSHCD_PLATFORM
>>  select SCSI_UFS_DWC_HOOKS
>>  help
>>This selects the DesignWare UFS host controller platform glue driver.
>>
>>Select this if you have a DesignWare UFS controller on Platform bus.
>>If unsure, say N.
>>
>> config SCSI_UFS_DWC_PCI
>>  tristate "DesignWare UFS controller pci glue driver"
>>  depends on SCSI_UFSHCD_PCI
>>  select SCSI_UFS_DWC_HOOKS
>>  help
>>This selects the DesignWare UFS host controller pci glue driver.
>>
>>Select this if you have a DesignWare UFS controller on pci bus.
>>If unsure, say N.
>>
>> config SCSI_UFS_DWC_TC
>>  bool "Support for the Synopsys Test Chip"
>>  depends on SCSI_UFS_DWC_HOOKS && (SCSI_UFSHCD_PCI || SCSI_UFS_DWC_PLAT)
>>  ---help---
>>Synopsys Test Chip is a Phy for prototyping purposes.
>>This selects the support for the Synopsys Test Chip.
>>
>>Select this if you have a Synopsys Test Chip.
>>If unsure, say N."
>>
>> Agree with the approach?
> 
> This would work, but I think it's better to define the options in terms
> of the top-level drivers, i.e. SCSI_UFS_DWC_TC_PCI and 
> SCSI_UFS_DWC_TC_PLATFORM,
> and then make the other options hidden and implicitly turned out by them.
> 

config SCSI_UFS_DWC
bool

config SCSI_UFS_DWC_TC_PLATFORM
tristate "DesignWare platform support using a G210 Test Chip"
depends on SCSI_UFSHCD_PLATFORM
select SCSI_UFS_DWC
---help---
  Synopsys Test Chip is a PHY for prototyping purposes.

  If unsure, say N."

config SCSI_UFS_DWC_TC_PCI
tristate "DesignWare pci support using a G210 Test Chip"
depends on SCSI_UFSHCD_PCI
select SCSI_UFS_DWC
---help---
  Synopsys Test Chip is a PHY for prototyping purposes.

  If unsure, say N."

I would keep SCSI_UFS_DWC to avoid building DWC specific source everytime.

Agree?

>   Arnd
> 

Joao
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] tcm_loop: use after free on error

[Tomas Henzl]

On 2.3.2016 11:09, Dan Carpenter wrote:
> We dereference "tl_nexus" to get the error code.
>
> Fixes: 1b418a8fcbc0 ('target: Convert demo-mode only drivers to 
> target_alloc_session')
> Signed-off-by: Dan Carpenter 

Reviewed-by: Tomas Henzl 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mvumi: fix build warning

[Johannes Thumshirn]

On Thu, Mar 03, 2016 at 06:36:53PM +0530, Sudip Mukherjee wrote:
> While building tilepro allmodconfig we were getting build warning:
> drivers/scsi/mvumi.c:2632:12: warning: 'mvumi_suspend' defined but not used
> drivers/scsi/mvumi.c:2651:12: warning: 'mvumi_resume' defined but not used
> 
> mvumi_suspend() and mvumi_resume() are only used when CONFIG_PM is
> defined as mentioned in the mvumi_pci_driver. 
> 
> Signed-off-by: Sudip Mukherjee 
> ---
>  drivers/scsi/mvumi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 02360de..d9d0736 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
> @@ -2629,6 +2629,7 @@ static void mvumi_shutdown(struct pci_dev *pdev)
>   mvumi_flush_cache(mhba);
>  }
>  
> +#ifdef CONFIG_PM
>  static int mvumi_suspend(struct pci_dev *pdev, pm_message_t state)
>  {
>   struct mvumi_hba *mhba = NULL;
> @@ -2716,6 +2717,7 @@ fail:
>  
>   return ret;
>  }
> +#endif
>  
>  static struct pci_driver mvumi_pci_driver = {
>  
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

There was a patch from Arnd earlier this day addressing this as well with a
__maybe_unused. You might want to have a look at this one
https://lkml.org/lkml/2016/3/2/497

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] mvumi: fix build warning

[Sudip Mukherjee]

While building tilepro allmodconfig we were getting build warning:
drivers/scsi/mvumi.c:2632:12: warning: 'mvumi_suspend' defined but not used
drivers/scsi/mvumi.c:2651:12: warning: 'mvumi_resume' defined but not used

mvumi_suspend() and mvumi_resume() are only used when CONFIG_PM is
defined as mentioned in the mvumi_pci_driver.   

Signed-off-by: Sudip Mukherjee 
---
 drivers/scsi/mvumi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index 02360de..d9d0736 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -2629,6 +2629,7 @@ static void mvumi_shutdown(struct pci_dev *pdev)
mvumi_flush_cache(mhba);
 }
 
+#ifdef CONFIG_PM
 static int mvumi_suspend(struct pci_dev *pdev, pm_message_t state)
 {
struct mvumi_hba *mhba = NULL;
@@ -2716,6 +2717,7 @@ fail:
 
return ret;
 }
+#endif
 
 static struct pci_driver mvumi_pci_driver = {
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 03/15] scsi: ufs: implement scsi host timeout handler

[Hannes Reinecke]

On 03/03/2016 05:10 PM, yga...@codeaurora.org wrote:
>> On 03/01/2016 09:25 PM, yga...@codeaurora.org wrote:
 On 02/28/2016 09:32 PM, Yaniv Gardi wrote:
> A race condition exists between request requeueing and scsi layer
> error handling:
> When UFS driver queuecommand returns a busy status for a request,
> it will be requeued and its tag will be freed and set to -1.
> At the same time it is possible that the request will timeout and
> scsi layer will start error handling for it. The scsi layer reuses
> the request and its tag to send error related commands to the device,
> however its tag is no longer valid.
 Hmm. How can the host return a 'busy' status for a request?
 From my understanding we have three possibilities:

 1) queuecommand returns busy; however, that means that the command has
 never been send and this issue shouldn't occur
 2) The command returns with BUSY status. But in this case it has
 already
 been returned, so there cannot be any timeout coming in.
 3) The host receives a command with a tag which is already in-use.
 However, that should have been prevented by the block-layer, which
 really should ensure that this situation never happens.

 So either way I look at it, it really looks like a bug and adding a
 timeout handler will just paper over it.
 (Not that a timeout handler is a bad idea, in fact I'm convinced that
 you need one. Just not for this purpose.)

 So can you elaborate how this 'busy' status comes about?
 Is the command sent to the device?

 Cheers,

 Hannes
>>>
>>>
>>> Hi Hannes,
>>>
>>> it's going to be a bit long :)
>>> I think you are missing the point.
>>> I will describe a race condition happened to us a while ago, that was
>>> quite difficult to understand and fix.
>>> So, this patch is not about the "busy" returning to the scsi dispatch
>>> routine. it's about the abort triggered after 30 seconds.
>>>
>>> imagine a request being queued and sent to the scsi, and then to the
>>> ufs.
>>> a timer, initialized to 30 seconds start ticking.
>>> but the request is never sent to the ufs device, as queuecommand()
>>> returns
>>> with "SCSI_MLQUEUE_HOST_BUSY"
>>> by looking at the code, this could happen, for example:
>>> err = ufshcd_hold(hba, true);
>>> if (err) {
>>> err = SCSI_MLQUEUE_HOST_BUSY;
>>> goto out;
>>> }
>>>
>> Uuhhh.
>> You probably should not have pointed me to that piece of code ...
>> open-coding loops in ufshcd_hold() ... shudder.
>> (Did I ever review that one? Must've ...)
>> _Anyway_: sleeping in queuecommand is always a bad idea, as then
>> precisely those issues you've just described will happen.
>>
>> Couldn't you just call
>> ufshcd_hold(hba, false)
>> instead of
>> ufshcd_hold(hba, true)
>> ?
>> The request will be requeued more-or-less immediately, avoiding the
>> issue with timeout handler kicking in.
>> And the queue will remain blocked until the ungate work item returns, at
>> which point I/O submission will continue.
>> As the request will be requeued to the head of the queue there won't be
>> other I/O competing with tags, so it shouldn't have any adverse effects.
>>
>> Wouldn't that work?
>>
>> Cheers,
>>
>> Hannes
> 
> Hi Hannes
> 
> This is a bug, and it should be fixed.
Oh, definitely agreed. The question is _where_.


> if you choose to bypass it, by calling ufshcd_hold(hba, false), not only
> the race condition is still there, and can pop-out at any other point in
> the future, but also, not sure what are the consequences of
> ufshcd_hold(hba, false) unstead of "true".
Well ... seeing it's your driver, I would've thought _you_ should know ...

> so, changing the already tested and working code, (not to return BUSY from
> queuecommand) is not a fix.
Hey, I did _not_ suggest not to retury BUSY from queuecommand.

I was suggesting this patch:

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 9c1b94b..b9295ad 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1388,7 +1388,7 @@ static int ufshcd_queuecommand(struct Scsi_Host
*host, struct scsi_cmnd *cmd)
goto out;
}

-   err = ufshcd_hold(hba, true);
+   err = ufshcd_hold(hba, false);
if (err) {
err = SCSI_MLQUEUE_HOST_BUSY;
clear_bit_unlock(tag, >lrb_in_use);

which, by reading the code, should be avoiding this issue.
I was just asking you if you could give this patch a spin and see if it
works. If not (for whatever reason) I'm happy to accept your patch.
But first I would like to have an explanation why the above would _not_
work.

Unfortunately I don't have the hardware otherwise I'd be running the
tests myself.

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. 

Re: [PATCH v9 3/3] add support for DWC UFS Host Controller

[Arnd Bergmann]

On Thursday 03 March 2016 11:39:05 Joao Pinto wrote:
> Hi Arnd,
> 
> On 3/2/2016 7:55 PM, Arnd Bergmann wrote:
> > On Wednesday 02 March 2016 16:46:47 Joao Pinto wrote:
> >> On 2/19/2016 3:03 PM, Arnd Bergmann wrote:
> >>> On Thursday 18 February 2016 17:20:27 Joao Pinto wrote:
> 
> Facts:
> 
> - Test Chip type are currently not detectable in runtime through the 
> controller
> - In the future the Test Chip version will be available in the controller
> - Test Chip initialization is different for each type
> - The IP Core version is 1.40a
> - Test Chip version is 6.00
> - Teh UFS version is 2.0

Ok.

> Suggested driver architecture:
> 
> Platform setup:
>  tc-dwc-g210-pltfrm --> tc-dwc-g210 --> ufshcd-dwc-pltfrm --> ufshcd-dwc --> 
> ufs
> 
> The test chip platform driver could be called through 2 compatibility strings.
> indicating the chip's version and bit type:
>  "snps, g210-tc-6.00-20bit"
>  "snps, g210-tc-6.00-40bit"

Yes, this sounds good. We can probably skip one of the middle layers,
but basically that is what I was looking for.

> The device tree node would have additional info compatibility strings as the 
> DWC
> IP core version and UFS version:
>  "snps, dwc-ufshcd-1.40a"
>  "jedec, ufs-2.0"
> 
> PCI based setup:
>  tc-dwc-g210-pci --> tc-dwc-g210 --> ufshcd-dwc-pci --> ufshcd-dwc --> ufs

The tc-dwc-g210 portion probably shouldn't depend on both
ufshcd-dwc-pltfrm and ufshcd-dwc-pci here, so how about leaving
those two out?


Then it becomes

   tc-dwc-g210-pci ---> tc-dwc-g210 --> ufshcd-dwc --> ufs
tc-dwc-g210-pltfrm --/

> The test chip type would be configured by a parameter to be passed in the 
> kernel
> boot args: tc_type = 20 (20-bit) or tc_type = 40 (40-bit)

Right. With module_param() helper, this will be either a boot command
line option, or a module load option, depending on whether the driver
is built-on or not.

modprobe tc-dwc-g210-pci tc_type=20

command line: tc-dwc-g210-pci.tc_type=20
 
> Having this in mind the KConfig would be:
> 
> "config SCSI_UFS_DWC_HOOKS
>   bool

I think we can now remove the config option for the hooks as well.

> config SCSI_UFS_DWC_PLAT
>   tristate "DesignWare UFS controller platform glue driver"
>   depends on SCSI_UFSHCD_PLATFORM
>   select SCSI_UFS_DWC_HOOKS
>   help
> This selects the DesignWare UFS host controller platform glue driver.
> 
> Select this if you have a DesignWare UFS controller on Platform bus.
> If unsure, say N.
> 
> config SCSI_UFS_DWC_PCI
>   tristate "DesignWare UFS controller pci glue driver"
>   depends on SCSI_UFSHCD_PCI
>   select SCSI_UFS_DWC_HOOKS
>   help
> This selects the DesignWare UFS host controller pci glue driver.
> 
> Select this if you have a DesignWare UFS controller on pci bus.
> If unsure, say N.
> 
> config SCSI_UFS_DWC_TC
>   bool "Support for the Synopsys Test Chip"
>   depends on SCSI_UFS_DWC_HOOKS && (SCSI_UFSHCD_PCI || SCSI_UFS_DWC_PLAT)
>   ---help---
> Synopsys Test Chip is a Phy for prototyping purposes.
> This selects the support for the Synopsys Test Chip.
> 
> Select this if you have a Synopsys Test Chip.
> If unsure, say N."
> 
> Agree with the approach?

This would work, but I think it's better to define the options in terms
of the top-level drivers, i.e. SCSI_UFS_DWC_TC_PCI and SCSI_UFS_DWC_TC_PLATFORM,
and then make the other options hidden and implicitly turned out by them.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 3/3] add support for DWC UFS Host Controller

[Joao Pinto]

Hi Arnd,

On 3/2/2016 7:55 PM, Arnd Bergmann wrote:
> On Wednesday 02 March 2016 16:46:47 Joao Pinto wrote:
>> On 2/19/2016 3:03 PM, Arnd Bergmann wrote:
>>> On Thursday 18 February 2016 17:20:27 Joao Pinto wrote:

Facts:

- Test Chip type are currently not detectable in runtime through the controller
- In the future the Test Chip version will be available in the controller
- Test Chip initialization is different for each type
- The IP Core version is 1.40a
- Test Chip version is 6.00
- Teh UFS version is 2.0

Suggested driver architecture:

Platform setup:
 tc-dwc-g210-pltfrm --> tc-dwc-g210 --> ufshcd-dwc-pltfrm --> ufshcd-dwc --> ufs

The test chip platform driver could be called through 2 compatibility strings.
indicating the chip's version and bit type:
 "snps, g210-tc-6.00-20bit"
 "snps, g210-tc-6.00-40bit"

The device tree node would have additional info compatibility strings as the DWC
IP core version and UFS version:
 "snps, dwc-ufshcd-1.40a"
 "jedec, ufs-2.0"

PCI based setup:
 tc-dwc-g210-pci --> tc-dwc-g210 --> ufshcd-dwc-pci --> ufshcd-dwc --> ufs

The test chip type would be configured by a parameter to be passed in the kernel
boot args: tc_type = 20 (20-bit) or tc_type = 40 (40-bit)

Having this in mind the KConfig would be:

"config SCSI_UFS_DWC_HOOKS
bool

config SCSI_UFS_DWC_PLAT
tristate "DesignWare UFS controller platform glue driver"
depends on SCSI_UFSHCD_PLATFORM
select SCSI_UFS_DWC_HOOKS
help
  This selects the DesignWare UFS host controller platform glue driver.

  Select this if you have a DesignWare UFS controller on Platform bus.
  If unsure, say N.

config SCSI_UFS_DWC_PCI
tristate "DesignWare UFS controller pci glue driver"
depends on SCSI_UFSHCD_PCI
select SCSI_UFS_DWC_HOOKS
help
  This selects the DesignWare UFS host controller pci glue driver.

  Select this if you have a DesignWare UFS controller on pci bus.
  If unsure, say N.

config SCSI_UFS_DWC_TC
bool "Support for the Synopsys Test Chip"
depends on SCSI_UFS_DWC_HOOKS && (SCSI_UFSHCD_PCI || SCSI_UFS_DWC_PLAT)
---help---
  Synopsys Test Chip is a Phy for prototyping purposes.
  This selects the support for the Synopsys Test Chip.

  Select this if you have a Synopsys Test Chip.
  If unsure, say N."

Agree with the approach?

Thanks for the help.

Joao

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 03/15] scsi: ufs: implement scsi host timeout handler

[ygardi]

> On 03/01/2016 09:25 PM, yga...@codeaurora.org wrote:
>>> On 02/28/2016 09:32 PM, Yaniv Gardi wrote:
 A race condition exists between request requeueing and scsi layer
 error handling:
 When UFS driver queuecommand returns a busy status for a request,
 it will be requeued and its tag will be freed and set to -1.
 At the same time it is possible that the request will timeout and
 scsi layer will start error handling for it. The scsi layer reuses
 the request and its tag to send error related commands to the device,
 however its tag is no longer valid.
>>> Hmm. How can the host return a 'busy' status for a request?
>>> From my understanding we have three possibilities:
>>>
>>> 1) queuecommand returns busy; however, that means that the command has
>>> never been send and this issue shouldn't occur
>>> 2) The command returns with BUSY status. But in this case it has
>>> already
>>> been returned, so there cannot be any timeout coming in.
>>> 3) The host receives a command with a tag which is already in-use.
>>> However, that should have been prevented by the block-layer, which
>>> really should ensure that this situation never happens.
>>>
>>> So either way I look at it, it really looks like a bug and adding a
>>> timeout handler will just paper over it.
>>> (Not that a timeout handler is a bad idea, in fact I'm convinced that
>>> you need one. Just not for this purpose.)
>>>
>>> So can you elaborate how this 'busy' status comes about?
>>> Is the command sent to the device?
>>>
>>> Cheers,
>>>
>>> Hannes
>>
>>
>> Hi Hannes,
>>
>> it's going to be a bit long :)
>> I think you are missing the point.
>> I will describe a race condition happened to us a while ago, that was
>> quite difficult to understand and fix.
>> So, this patch is not about the "busy" returning to the scsi dispatch
>> routine. it's about the abort triggered after 30 seconds.
>>
>> imagine a request being queued and sent to the scsi, and then to the
>> ufs.
>> a timer, initialized to 30 seconds start ticking.
>> but the request is never sent to the ufs device, as queuecommand()
>> returns
>> with "SCSI_MLQUEUE_HOST_BUSY"
>> by looking at the code, this could happen, for example:
>>  err = ufshcd_hold(hba, true);
>>  if (err) {
>>  err = SCSI_MLQUEUE_HOST_BUSY;
>>  goto out;
>>  }
>>
> Uuhhh.
> You probably should not have pointed me to that piece of code ...
> open-coding loops in ufshcd_hold() ... shudder.
> (Did I ever review that one? Must've ...)
> _Anyway_: sleeping in queuecommand is always a bad idea, as then
> precisely those issues you've just described will happen.
>
> Couldn't you just call
> ufshcd_hold(hba, false)
> instead of
> ufshcd_hold(hba, true)
> ?
> The request will be requeued more-or-less immediately, avoiding the
> issue with timeout handler kicking in.
> And the queue will remain blocked until the ungate work item returns, at
> which point I/O submission will continue.
> As the request will be requeued to the head of the queue there won't be
> other I/O competing with tags, so it shouldn't have any adverse effects.
>
> Wouldn't that work?
>
> Cheers,
>
> Hannes

Hi Hannes

This is a bug, and it should be fixed.
if you choose to bypass it, by calling ufshcd_hold(hba, false), not only
the race condition is still there, and can pop-out at any other point in
the future, but also, not sure what are the consequences of
ufshcd_hold(hba, false) unstead of "true".
so, changing the already tested and working code, (not to return BUSY from
queuecommand) is not a fix.
I strongly recommend we upstream this race-condition fix.

thanks,
Yaniv



> --
> 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-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 5/6] scsi_dh_emc: update 'access_state' field

[Johannes Thumshirn]

On Thu, Mar 03, 2016 at 07:54:11AM +0100, Hannes Reinecke wrote:
> Update the 'access_state' field of the SCSI device whenever
> the path state changes.
> 
> Reviewed-by: Christoph Hellwig 
> Signed-off-by: Hannes Reinecke 

Reviewed-by: Johannes Thumshirn 

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 6/6] scsi_sysfs: call 'device_add' after attaching device handler

[Johannes Thumshirn]

On Thu, Mar 03, 2016 at 07:54:12AM +0100, Hannes Reinecke wrote:
> 'device_add' will be evaluating the 'is_visible' callback
> when creating the sysfs attributes. As by this time the
> device handler has not been attached the 'access_state'
> attribute will never be visible.
> 
> This patch moves the code around so that the device handler
> is present by the time 'is_visible' is evaluated to
> correctly display the 'access_state' attribute.
> 
> Reviewed-by: Christoph Hellwig 
> Signed-off-by: Hannes Reinecke 

Reviewed-by: Johannes Thumshirn 

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 4/6] scsi_dh_rdac: update 'access_state' field

[Johannes Thumshirn]

On Thu, Mar 03, 2016 at 07:54:10AM +0100, Hannes Reinecke wrote:
> Track attached SCSI devices and update the 'access_state'
> whenever the path state of the device changes.
> 
> Reviewed-by: Christoph Hellwig 
> Signed-off-by: Hannes Reinecke 

Reviewed-by: Johannes Thumshirn 

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 3/6] scsi_dh_alua: update 'access_state' field

[Johannes Thumshirn]

On Thu, Mar 03, 2016 at 07:54:09AM +0100, Hannes Reinecke wrote:
> Track attached SCSI devices and update the 'access_state' field
> whenever an ALUA state change has been detected.
> 
> Reviewed-by: Bart Van Assche 
> Reviewed-by: Ewan Milne 
> Reviewed-by: Christoph Hellwig 
> Signed-off-by: Hannes Reinecke 

Reviewed-by: Johannes Thumshirn 

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 2/6] scsi_dh_alua: use common definitions for ALUA state

[Johannes Thumshirn]

On Thu, Mar 03, 2016 at 07:54:08AM +0100, Hannes Reinecke wrote:
> scsi_proto.h now contains definitions for the ALUA state,
> so we don't have to carry them in the device handler.
> 
> Reviewed-by: Bart van Assche 
> Reviewed-by: Christoph Hellwig 
> Signed-off-by: Hannes Reinecke 

Reviewed-by: Johannes Thumshirn 

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 1/6] scsi: Add 'access_state' and 'preferred_path' attribute

[Johannes Thumshirn]

On Thu, Mar 03, 2016 at 07:54:07AM +0100, Hannes Reinecke wrote:
> Add an 'access_state' field to struct scsi_device
> and display them in sysfs as 'access_state' and
> 'preferred_path' attribute.
> 
> Reviewed-by: Christoph Hellwig 
> Signed-off-by: Hannes Reinecke 

Reviewed-by: Johannes Thumshirn 

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] scsi_sysfs: add 'is_bin_visible' callback

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig 
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/14] scsi: mvumi: use __maybe_unused to hide pm functions

[Johannes Thumshirn]

On Wed, Mar 02, 2016 at 04:59:00PM +0100, Arnd Bergmann wrote:
> The mvumi scsi hides the references to its suspend/resume functions
> in an #ifdef but does not hide the implementation the same way:
> 
> drivers/scsi/mvumi.c:2632:12: error: 'mvumi_suspend' defined but not used 
> [-Werror=unused-function]
> drivers/scsi/mvumi.c:2651:12: error: 'mvumi_resume' defined but not used 
> [-Werror=unused-function]
> 
> This adds __maybe_unused annotations so the compiler knows
> it can silently drop them instead of warning, while avoiding
> the addition of another #ifdef.
> 
> Signed-off-by: Arnd Bergmann 

Reviewed-by: Johannes Thumshirn 

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html