Re: [PATCH v2 3/7] drivers/base: Probe devices concurrently if requested by the driver

2018-10-18 Thread Alexander Duyck 




On 10/18/2018 10:15 AM, Greg KH wrote:

On Thu, Oct 18, 2018 at 09:51:03AM -0700, Alexander Duyck wrote:

On 10/18/2018 9:46 AM, Bart Van Assche wrote:

On Thu, 2018-10-18 at 08:25 -0700, Alexander Duyck wrote:

On 10/17/2018 5:54 PM, Dan Williams wrote:

On Wed, Oct 17, 2018 at 4:41 PM Bart Van Assche  wrote:


Instead of probing devices sequentially in the PROBE_PREFER_ASYNCHRONOUS
mode, scan devices concurrently. This helps when the wall clock time for
a single probe is significantly above the CPU time needed for a single
probe, e.g. when scanning SCSI LUNs over a storage network.


Alex had a similar patch here [1] that I don't think has been accepted
yet, in any event some collaboration is needed:

[1]: https://lkml.org/lkml/2018/9/27/14


The patch set referenced is a little out of date. The latest set is:
https://lore.kernel.org/lkml/20181015150305.29520.86363.stgit@localhost.localdomain/

I'm also not quite sure what the point of this patch is. I don't think
it is doing what it says it is doing. From what I can tell it is just
allowing the driver init code to ignore if the driver wants to be probed
asynchronously or not. Further comments inline below.


Hi Alexander,

Thanks for the pointer to the latest version of your patch series. I was not
yet aware of your work when I posted this patch series. Now that I have had a
look at your patch series I like your approach better than what I did in this
patch. Since it could take a while before agreement is reached about the async
domain patches in the same patch series, how about you submitting patches 3/6
and 4/6 from your patch series to Greg for kernel version v4.20? If I drop
the driver core patches from my patch series and replace these with your
driver core patches I achieve the same results. If you Cc me when you resubmit
these patches I will review them.

Thanks,

Bart.


Actually the async and workqueue patches have already been reviewed and last
I knew they were okay with the workqueue guys. These patches are already
submitted to Greg for 4.20.


It's too late for 4.20 now, sorry.  They will have to wait.  Given that
4.19-final could have come out last weekend, this shouldn't be a
supprise.

They are in my review queue and I'll get to them after 4.20-rc1 is out.

thanks,

greg k-h


Well I was hoping for 4.20 anyway. :-/

Thanks for letting me know.

- Alex

Re: [PATCH v2 3/7] drivers/base: Probe devices concurrently if requested by the driver

2018-10-18 Thread Alexander Duyck 

On 10/18/2018 9:46 AM, Bart Van Assche wrote:

On Thu, 2018-10-18 at 08:25 -0700, Alexander Duyck wrote:

On 10/17/2018 5:54 PM, Dan Williams wrote:

On Wed, Oct 17, 2018 at 4:41 PM Bart Van Assche  wrote:


Instead of probing devices sequentially in the PROBE_PREFER_ASYNCHRONOUS
mode, scan devices concurrently. This helps when the wall clock time for
a single probe is significantly above the CPU time needed for a single
probe, e.g. when scanning SCSI LUNs over a storage network.


Alex had a similar patch here [1] that I don't think has been accepted
yet, in any event some collaboration is needed:

[1]: https://lkml.org/lkml/2018/9/27/14


The patch set referenced is a little out of date. The latest set is:
https://lore.kernel.org/lkml/20181015150305.29520.86363.stgit@localhost.localdomain/

I'm also not quite sure what the point of this patch is. I don't think
it is doing what it says it is doing. From what I can tell it is just
allowing the driver init code to ignore if the driver wants to be probed
asynchronously or not. Further comments inline below.


Hi Alexander,

Thanks for the pointer to the latest version of your patch series. I was not
yet aware of your work when I posted this patch series. Now that I have had a
look at your patch series I like your approach better than what I did in this
patch. Since it could take a while before agreement is reached about the async
domain patches in the same patch series, how about you submitting patches 3/6
and 4/6 from your patch series to Greg for kernel version v4.20? If I drop
the driver core patches from my patch series and replace these with your
driver core patches I achieve the same results. If you Cc me when you resubmit
these patches I will review them.

Thanks,

Bart.


Actually the async and workqueue patches have already been reviewed and 
last I knew they were okay with the workqueue guys. These patches are 
already submitted to Greg for 4.20.


The only bits that I had left were the patches for driver-core and I am 
already working those through with Rafael Wysocki and Greg KH in terms 
of addressing their review comments.


Thanks.

- Alex

Re: [PATCH v2 3/7] drivers/base: Probe devices concurrently if requested by the driver

2018-10-18 Thread Alexander Duyck 

On 10/17/2018 5:54 PM, Dan Williams wrote:

On Wed, Oct 17, 2018 at 4:41 PM Bart Van Assche  wrote:


Instead of probing devices sequentially in the PROBE_PREFER_ASYNCHRONOUS
mode, scan devices concurrently. This helps when the wall clock time for
a single probe is significantly above the CPU time needed for a single
probe, e.g. when scanning SCSI LUNs over a storage network.


Alex had a similar patch here [1] that I don't think has been accepted
yet, in any event some collaboration is needed:

[1]: https://lkml.org/lkml/2018/9/27/14


The patch set referenced is a little out of date. The latest set is:
https://lore.kernel.org/lkml/20181015150305.29520.86363.stgit@localhost.localdomain/

I'm also not quite sure what the point of this patch is. I don't think 
it is doing what it says it is doing. From what I can tell it is just 
allowing the driver init code to ignore if the driver wants to be probed 
asynchronously or not. Further comments inline below.






Cc: Lee Duncan 
Cc: Hannes Reinecke 
Cc: Luis Chamberlain 
Cc: Johannes Thumshirn 
Cc: Christoph Hellwig 
Cc: Greg Kroah-Hartman 
Cc: Dan Williams 
Signed-off-by: Bart Van Assche 
---
  drivers/base/bus.c |  3 +--
  drivers/base/dd.c  | 49 ++
  2 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 8bfd27ec73d6..18ca1178821f 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -696,8 +696,7 @@ int bus_add_driver(struct device_driver *drv)

  out_unregister:
 kobject_put(>kobj);
-   /* drv->p is freed in driver_release()  */
-   drv->p = NULL;
+
  out_put_bus:
 bus_put(bus);
 return error;
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 033382421351..f8d645aa09be 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -27,6 +27,7 @@
  #include 
  #include 
  #include 
+#include 

  #include "base.h"
  #include "power/power.h"
@@ -691,6 +692,49 @@ int driver_probe_device(struct device_driver *drv, struct 
device *dev)
 return ret;
  }

+struct driver_and_dev {
+   struct device_driver*drv;
+   struct device   *dev;
+};
+
+static void __driver_probe_device_async(void *data, async_cookie_t cookie)
+{
+   struct driver_and_dev *dd = data;
+   struct device_driver *drv = dd->drv;
+   struct device *dev = dd->dev;
+
+   device_lock(dev);
+   driver_probe_device(drv, dev);
+   device_unlock(dev);
+   kobject_put(>p->kobj);
+   module_put(drv->owner);
+   kfree(dd);
+}
+
+static void driver_probe_device_async(struct device_driver *drv,
+ struct device *dev)
+{
+   struct driver_and_dev *dd;
+
+   if (!try_module_get(drv->owner))
+   return;
+   dd = kmalloc(sizeof(*dd), GFP_KERNEL);
+   if (!dd) {
+   /* If out of memory, scan synchronously. */
+   device_lock(dev);
+   driver_probe_device(drv, dev);
+   device_unlock(dev);
+   module_put(drv->owner);
+   return;
+   }
+   *dd = (struct driver_and_dev){
+   .drv = drv,
+   .dev = dev,
+   };
+   kobject_get(>p->kobj);
+   async_schedule(__driver_probe_device_async, dd);
+}
+


So this piece is similar to what I had, but the functionality is being 
used in a completely different spot. I'm not entirely convinced this 
isn't redundant.



  bool driver_allows_async_probing(struct device_driver *drv)
  {
 switch (drv->probe_type) {
@@ -777,6 +821,11 @@ static int __device_attach_driver(struct device_driver 
*drv, void *_data)
 if (data->check_async && async_allowed != data->want_async)
 return 0;

+   if (data->check_async) {
+   driver_probe_device_async(drv, dev);
+   return 0;
+   }
+


So this code path assumes the driver is already loaded before the device 
is added, and from what I can tell it looks like it is forcing 
everything to asynchronously probe isn't it? Wasn't that the purpose of 
the async_allowed != data->want_async check?


Also this seems redundant since the return 0 case here is supposed to 
have us attach the device asynchronously. So it seems like we are just 
adding another layer of aysnc init. It seems like the most direct way of 
doing this would be to just force data->want_async to always be true by 
passing that instead of false in device_attach?



 return driver_probe_device(drv, dev);
  }

--
2.19.1.568.g152ad8e336-goog

Re: [PATCH 0/2] scsi: Fix endless loop of ATA hard resets due to VPD reads

2016-02-03 Thread Alexander Duyck 
On Tue, Feb 2, 2016 at 10:48 PM, Kirill A. Shutemov
 wrote:
> On Tue, Feb 02, 2016 at 09:45:48PM -0500, Martin K. Petersen wrote:
>> > "Kirill" == Kirill A Shutemov  writes:
>>
>> Kirill> I have the same problem.
>>
>> Kirill> Shouldn't we put quirk for that?
>>
>> I was hoping that Hannes' patch would do the trick so we could avoid
>> blacklisting:
>>
>>   https://patchwork.kernel.org/patch/8079011/
>
> It didn't help me.

My patch didn't resolve the Marvell issue, it just resolved other
cases that could also lead to the same problem.

I added a line to my kernel parameters to blacklist the Marvell
console.  I wasn't sure if blacklisting would have been acceptable.
That is why I called out that I still had to add that line to my
kernel in the cover letter for my patch set.

- Alex
--
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 1/2] scsi: Do not attach VPD to devices that don't support it

2016-01-21 Thread Alexander Duyck 
On Wed, Jan 20, 2016 at 11:37 PM, Hannes Reinecke <h...@suse.de> wrote:
> On 01/21/2016 07:35 AM, Alexander Duyck wrote:
>> The patch "scsi: rescan VPD attributes" introduced a regression in which
>> devices that don't support VPD were being scanned for VPD attributes
>> anyway.  This could cause issues for this parts and should be avoided so
>> the check for scsi_level has been moved out of scsi_add_lun and into
>> scsi_attach_vpd so that all callers will not scan VPD for devices that
>> don't support it.
>>
>> Fixes: 09e2b0b14690 ("scsi: rescan VPD attributes")
>> Signed-off-by: Alexander Duyck <adu...@mirantis.com>
>> ---
>>  drivers/scsi/scsi.c  |3 +++
>>  drivers/scsi/scsi_scan.c |3 +--
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
>> index b1bf42b93fcc..ed085e78c893 100644
>> --- a/drivers/scsi/scsi.c
>> +++ b/drivers/scsi/scsi.c
>> @@ -784,6 +784,9 @@ void scsi_attach_vpd(struct scsi_device *sdev)
>>   int pg83_supported = 0;
>>   unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
>>
>> + if (sdev->scsi_level < SCSI_3)
>> + return;
>> +
>>   if (sdev->skip_vpd_pages)
>>   return;
>>  retry_pg0:
>> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
>> index 6a820668d442..1b16c89e0cf9 100644
>> --- a/drivers/scsi/scsi_scan.c
>> +++ b/drivers/scsi/scsi_scan.c
>> @@ -986,8 +986,7 @@ static int scsi_add_lun(struct scsi_device *sdev, 
>> unsigned char *inq_result,
>>   }
>>   }
>>
>> - if (sdev->scsi_level >= SCSI_3)
>> - scsi_attach_vpd(sdev);
>> + scsi_attach_vpd(sdev);
>>
>>   sdev->max_queue_depth = sdev->queue_depth;
>>
>>
> Isn't this slightly pointless, given that we're testing the inverse
> condition in scsi_attach_vpd()?

I'm not sure what you are getting at.  What I basically did is move
the check here into the function.  No point in checking it in 2 spots
when checking it inside the function is good enough.

> And in anycase, I guess we should be using the same logic sd.c is
> using. Please see the attached patch.

The attached patch looks good as it also takes care of the opt-in case
which I had overlooked.  The only bit missing is the fact that we are
still checking scsi_level twice when we don't need to.

- Alex
--
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 0/2] scsi: Fix endless loop of ATA hard resets due to VPD reads

2016-01-20 Thread Alexander Duyck 
Recent changes to the kernel pulled in during the merge window have
resulted in my system generating an endless loop of the following type of
errors:

[  318.965756] ata14: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
[  318.968457] ata14.00: configured for UDMA/66
[  318.970656] ata14: EH complete
[  318.984366] ata14.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6
[  318.986854] ata14.00: irq_stat 0x4001
[  318.989138] ata14.00: cmd a0/01:00:00:00:01/00:00:00:00:00/a0 tag 22 dma 
16640 in
 Inquiry 12 01 00 00 ff 00res 00/00:00:00:00:00/00:00:00:00:00/00 Emask 
0x3 (HSM violation)
[  318.995986] ata14: hard resetting link

I bisected the issue and found the patch responsible for the issue was
commit 09e2b0b14690 "scsi: rescan VPD attributes".  This commit contained
several issues.

First, the commit had changed the behavior in terms of what devices we
called scsi_attach_vpd() for.  As a result we were calling it for devices
that didn't support a scsi_level of 6, SCSI 3, so VPD accesses could
result in errors.

Second, the commit as well as a follow-on patch for it contained a number
of RCU errors.  Specifically the code was structured such that we had
accesses outside of RCU locked regions, and repeated use of the RCU
protected pointer without using the proper accessors.  As such it was
possible to get into a serious corruption situation should a pointer be
updated.

Ultimately neither of these bugs were my root cause.  It turns out the
Marvel Console SCSI device in my system needed to have a flag set to
disable VPD access in order to keep things from looping through the error
repeatedly.  In order to resolve it I had to add the kernel parameter
"scsi_mod.dev_flags=Marvell:Console:0x400".  This allowed my system to
boot without any errors, however the first two issues described above are
still relevent so I thought I would provide the patches since I had already
written them up.

---

Alexander Duyck (2):
  scsi: Do not attach VPD to devices that don't support it
  scsi: Fix RCU handling for VPD pages


 drivers/scsi/scsi.c|   55 
 drivers/scsi/scsi_lib.c|   12 +-
 drivers/scsi/scsi_scan.c   |3 +-
 drivers/scsi/scsi_sysfs.c  |   14 ++-
 include/scsi/scsi_device.h |   14 +++
 5 files changed, 54 insertions(+), 44 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

[PATCH 1/2] scsi: Do not attach VPD to devices that don't support it

2016-01-20 Thread Alexander Duyck 
The patch "scsi: rescan VPD attributes" introduced a regression in which
devices that don't support VPD were being scanned for VPD attributes
anyway.  This could cause issues for this parts and should be avoided so
the check for scsi_level has been moved out of scsi_add_lun and into
scsi_attach_vpd so that all callers will not scan VPD for devices that
don't support it.

Fixes: 09e2b0b14690 ("scsi: rescan VPD attributes")
Signed-off-by: Alexander Duyck <adu...@mirantis.com>
---
 drivers/scsi/scsi.c  |3 +++
 drivers/scsi/scsi_scan.c |3 +--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index b1bf42b93fcc..ed085e78c893 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -784,6 +784,9 @@ void scsi_attach_vpd(struct scsi_device *sdev)
int pg83_supported = 0;
unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
 
+   if (sdev->scsi_level < SCSI_3)
+   return;
+
if (sdev->skip_vpd_pages)
return;
 retry_pg0:
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 6a820668d442..1b16c89e0cf9 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -986,8 +986,7 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned 
char *inq_result,
}
}
 
-   if (sdev->scsi_level >= SCSI_3)
-   scsi_attach_vpd(sdev);
+   scsi_attach_vpd(sdev);
 
sdev->max_queue_depth = sdev->queue_depth;
 

--
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 2/2] scsi: Fix RCU handling for VPD pages

2016-01-20 Thread Alexander Duyck 
This patch is meant to fix the RCU handling for VPD pages.  The original
code had a number of issues including the fact that the local variables
were being declared as __rcu, the RCU variable being directly accessed
outside of the RCU locked region, and the fact that length was not
associated with the data so it would be possible to get a mix and match of
the length for one VPD page with the data from another.

Fixes: 09e2b0b14690 ("scsi: rescan VPD attributes")
Signed-off-by: Alexander Duyck <adu...@mirantis.com>
---
 drivers/scsi/scsi.c|   52 +++-
 drivers/scsi/scsi_lib.c|   12 +-
 drivers/scsi/scsi_sysfs.c  |   14 +++-
 include/scsi/scsi_device.h |   14 
 4 files changed, 50 insertions(+), 42 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index ed085e78c893..143b384fd145 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -782,7 +782,7 @@ void scsi_attach_vpd(struct scsi_device *sdev)
int vpd_len = SCSI_VPD_PG_LEN;
int pg80_supported = 0;
int pg83_supported = 0;
-   unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
+   unsigned char *vpd_buf;
 
if (sdev->scsi_level < SCSI_3)
return;
@@ -816,58 +816,60 @@ retry_pg0:
vpd_len = SCSI_VPD_PG_LEN;
 
if (pg80_supported) {
+   struct scsi_vpd_pg *vpd, *orig_vpd;
 retry_pg80:
-   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
-   if (!vpd_buf)
+   vpd = kmalloc(sizeof(*vpd) + vpd_len, GFP_KERNEL);
+   if (!vpd)
return;
 
-   result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
+   result = scsi_vpd_inquiry(sdev, vpd->buf, 0x80, vpd_len);
if (result < 0) {
-   kfree(vpd_buf);
+   kfree(vpd);
return;
}
if (result > vpd_len) {
vpd_len = result;
-   kfree(vpd_buf);
+   kfree(vpd);
goto retry_pg80;
}
+   vpd->len = result;
+
mutex_lock(>inquiry_mutex);
-   orig_vpd_buf = sdev->vpd_pg80;
-   sdev->vpd_pg80_len = result;
-   rcu_assign_pointer(sdev->vpd_pg80, vpd_buf);
+   orig_vpd = rcu_dereference_protected(sdev->vpd_pg80, 1);
+   rcu_assign_pointer(sdev->vpd_pg80, vpd);
mutex_unlock(>inquiry_mutex);
-   synchronize_rcu();
-   if (orig_vpd_buf) {
-   kfree(orig_vpd_buf);
-   orig_vpd_buf = NULL;
-   }
+
+   if (orig_vpd)
+   kfree_rcu(orig_vpd, rcu);
vpd_len = SCSI_VPD_PG_LEN;
}
 
if (pg83_supported) {
+   struct scsi_vpd_pg *vpd, *orig_vpd;
 retry_pg83:
-   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
-   if (!vpd_buf)
+   vpd = kmalloc(sizeof(*vpd) + vpd_len, GFP_KERNEL);
+   if (!vpd)
return;
 
-   result = scsi_vpd_inquiry(sdev, vpd_buf, 0x83, vpd_len);
+   result = scsi_vpd_inquiry(sdev, vpd->buf, 0x83, vpd_len);
if (result < 0) {
-   kfree(vpd_buf);
+   kfree(vpd);
return;
}
if (result > vpd_len) {
vpd_len = result;
-   kfree(vpd_buf);
+   kfree(vpd);
goto retry_pg83;
}
+   vpd->len = result;
+
mutex_lock(>inquiry_mutex);
-   orig_vpd_buf = sdev->vpd_pg83;
-   sdev->vpd_pg83_len = result;
-   rcu_assign_pointer(sdev->vpd_pg83, vpd_buf);
+   orig_vpd = rcu_dereference_protected(sdev->vpd_pg83, 1);
+   rcu_assign_pointer(sdev->vpd_pg83, vpd);
mutex_unlock(>inquiry_mutex);
-   synchronize_rcu();
-   if (orig_vpd_buf)
-   kfree(orig_vpd_buf);
+
+   if (orig_vpd)
+   kfree_rcu(orig_vpd, rcu);
}
 }
 
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index fa6b2c4eb7a2..e44f66bc4c90 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3175,7 +3175,7 @@ int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, 
size_t id_len)
u8 cur_id_type = 0xff;
u8 cur_id_size = 0;
unsigned char *d, *cur_id_str;
-   unsigned char __rcu *vpd_pg83;
+   struct scsi_vpd_pg *vpd_pg83;
int id_size = -EINVAL;
 
rcu_read_lock();
@@ -3205,8 +3205,8 @@ int scsi_vpd_lun_id(struct scsi_devi