Re: [Qemu-devel] virtio-console downgrade the virtio-pci-blk performance

2018-10-01 Thread Dr. David Alan Gilbert
* Feng Li (lifeng1...@gmail.com) wrote:
> Hi,
> I found an obvious performance downgrade when virtio-console combined
> with virtio-pci-blk.
> 
> This phenomenon exists in nearly all Qemu versions and all Linux
> (CentOS7, Fedora 28, Ubuntu 18.04) distros.
> 
> This is a disk cmd:
> -drive 
> file=iscsi://127.0.0.1:3260/iqn.2016-02.com.test:system:fl-iscsi/1,format=raw,if=none,id=drive-virtio-disk0,cache=none,aio=native
> -device 
> virtio-blk-pci,scsi=off,bus=pci.0,addr=0x6,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on
> 
> If I add "-device
> virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x5  ", the virtio
> disk 4k iops (randread/randwrite) would downgrade from 60k to 40k.
> 
> In VM, if I rmmod virtio-console, the performance will back to normal.
> 
> Any idea about this issue?
> 
> I don't know this is a qemu issue or kernel issue.

It sounds odd;  can you provide more details on:
  a) The benchmark you're using.
  b) the host and the guest config (number of cpus etc)
  c) Why are you running it with iscsi back to the same host - why not
 just simplify the test back to a simple file?

Dave

> 
> Thanks in advance.
> -- 
> Thanks and Best Regards,
> Alex
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK


[PATCH] Revert "genirq/affinity: assign vectors to all possible CPUs"

2018-10-01 Thread Paul Menzel
Date: Wed, 29 Aug 2018 17:28:45 +0200

This reverts commit ef86f3a72adb8a7931f67335560740a7ad696d1d.

See the discussion in the thread *aacraid: Regression in 4.14.56
with *genirq/affinity: assign vectors to all possible CPUs** on
the linux-scsi list.

Reverting the commit, the aacraid driver loads correctly, and the
drives are visible. So revert the commit to adhere to Linux’ no
regression policy.

Strangely, the issue is not reproducible with Linux 4.18.x, but
Ming Lei wrote, that this might only be by chance.

Link: https://www.spinics.net/lists/linux-scsi/msg122732.html
Signed-off-by: Paul Menzel 

---
 kernel/irq/affinity.c | 30 +++---
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/kernel/irq/affinity.c b/kernel/irq/affinity.c
index a37a3b4b6342..e12d35108225 100644
--- a/kernel/irq/affinity.c
+++ b/kernel/irq/affinity.c
@@ -39,7 +39,7 @@ static void irq_spread_init_one(struct cpumask *irqmsk, 
struct cpumask *nmsk,
}
 }
 
-static cpumask_var_t *alloc_node_to_possible_cpumask(void)
+static cpumask_var_t *alloc_node_to_present_cpumask(void)
 {
cpumask_var_t *masks;
int node;
@@ -62,7 +62,7 @@ static cpumask_var_t *alloc_node_to_possible_cpumask(void)
return NULL;
 }
 
-static void free_node_to_possible_cpumask(cpumask_var_t *masks)
+static void free_node_to_present_cpumask(cpumask_var_t *masks)
 {
int node;
 
@@ -71,22 +71,22 @@ static void free_node_to_possible_cpumask(cpumask_var_t 
*masks)
kfree(masks);
 }
 
-static void build_node_to_possible_cpumask(cpumask_var_t *masks)
+static void build_node_to_present_cpumask(cpumask_var_t *masks)
 {
int cpu;
 
-   for_each_possible_cpu(cpu)
+   for_each_present_cpu(cpu)
cpumask_set_cpu(cpu, masks[cpu_to_node(cpu)]);
 }
 
-static int get_nodes_in_cpumask(cpumask_var_t *node_to_possible_cpumask,
+static int get_nodes_in_cpumask(cpumask_var_t *node_to_present_cpumask,
const struct cpumask *mask, nodemask_t *nodemsk)
 {
int n, nodes = 0;
 
/* Calculate the number of nodes in the supplied affinity mask */
for_each_node(n) {
-   if (cpumask_intersects(mask, node_to_possible_cpumask[n])) {
+   if (cpumask_intersects(mask, node_to_present_cpumask[n])) {
node_set(n, *nodemsk);
nodes++;
}
@@ -109,7 +109,7 @@ irq_create_affinity_masks(int nvecs, const struct 
irq_affinity *affd)
int last_affv = affv + affd->pre_vectors;
nodemask_t nodemsk = NODE_MASK_NONE;
struct cpumask *masks;
-   cpumask_var_t nmsk, *node_to_possible_cpumask;
+   cpumask_var_t nmsk, *node_to_present_cpumask;
 
/*
 * If there aren't any vectors left after applying the pre/post
@@ -125,8 +125,8 @@ irq_create_affinity_masks(int nvecs, const struct 
irq_affinity *affd)
if (!masks)
goto out;
 
-   node_to_possible_cpumask = alloc_node_to_possible_cpumask();
-   if (!node_to_possible_cpumask)
+   node_to_present_cpumask = alloc_node_to_present_cpumask();
+   if (!node_to_present_cpumask)
goto out;
 
/* Fill out vectors at the beginning that don't need affinity */
@@ -135,8 +135,8 @@ irq_create_affinity_masks(int nvecs, const struct 
irq_affinity *affd)
 
/* Stabilize the cpumasks */
get_online_cpus();
-   build_node_to_possible_cpumask(node_to_possible_cpumask);
-   nodes = get_nodes_in_cpumask(node_to_possible_cpumask, 
cpu_possible_mask,
+   build_node_to_present_cpumask(node_to_present_cpumask);
+   nodes = get_nodes_in_cpumask(node_to_present_cpumask, cpu_present_mask,
 &nodemsk);
 
/*
@@ -146,7 +146,7 @@ irq_create_affinity_masks(int nvecs, const struct 
irq_affinity *affd)
if (affv <= nodes) {
for_each_node_mask(n, nodemsk) {
cpumask_copy(masks + curvec,
-node_to_possible_cpumask[n]);
+node_to_present_cpumask[n]);
if (++curvec == last_affv)
break;
}
@@ -160,7 +160,7 @@ irq_create_affinity_masks(int nvecs, const struct 
irq_affinity *affd)
vecs_per_node = (affv - (curvec - affd->pre_vectors)) / nodes;
 
/* Get the cpus on this node which are in the mask */
-   cpumask_and(nmsk, cpu_possible_mask, 
node_to_possible_cpumask[n]);
+   cpumask_and(nmsk, cpu_present_mask, node_to_present_cpumask[n]);
 
/* Calculate the number of cpus per vector */
ncpus = cpumask_weight(nmsk);
@@ -192,7 +192,7 @@ irq_create_affinity_masks(int nvecs, const struct 
irq_affinity *affd)
/* Fill out vectors at the end that don't need affinity */
for (; curvec < nvecs; curvec++)
   

Re: [PATCH] Revert "genirq/affinity: assign vectors to all possible CPUs"

2018-10-01 Thread Christoph Hellwig
On Mon, Oct 01, 2018 at 02:33:07PM +0200, Paul Menzel wrote:
> Date: Wed, 29 Aug 2018 17:28:45 +0200
> 
> This reverts commit ef86f3a72adb8a7931f67335560740a7ad696d1d.

This seems rather odd.  If add all you'd revert the patch adding the
PCI_IRQ_AFFINITY to aacraid, not core infrastructure.


Re: [PATCH] Revert "genirq/affinity: assign vectors to all possible CPUs"

2018-10-01 Thread Paul Menzel
Dear Christoph,


On 10/01/18 14:35, Christoph Hellwig wrote:
> On Mon, Oct 01, 2018 at 02:33:07PM +0200, Paul Menzel wrote:
>> Date: Wed, 29 Aug 2018 17:28:45 +0200
>>
>> This reverts commit ef86f3a72adb8a7931f67335560740a7ad696d1d.
> 
> This seems rather odd.  If at all you'd revert the patch adding the
> PCI_IRQ_AFFINITY to aacraid, not core infrastructure.

Thank you for the suggestion, but that flag was added in 2016
to the aacraid driver.

> commit 0910d8bbdd99856af1394d3d8830955abdefee4a
> Author: Hannes Reinecke 
> Date:   Tue Nov 8 08:11:30 2016 +0100
> 
> scsi: aacraid: switch to pci_alloc_irq_vectors
> 
> Use pci_alloc_irq_vectors and drop the hand-crafted interrupt affinity
> routines.

So what would happen, if `PCI_IRQ_AFFINITY` was removed? Will the
system still work with the same performance?

As far as I understood, the no regression policy is there for
exactly that reason, and it shouldn’t matter if it’s core
infrastructure or not. As written, I have no idea, and just know
reverting the commit in question fixes the problem here. So I’ll
gladly test other solutions to fix this issue.


Kind regards,

Paul



smime.p7s
Description: S/MIME Cryptographic Signature


LED trigger for UFS device activity

2018-10-01 Thread Manivannan Sadhasivam
Hello everyone,

I'm looking into adding LED trigger support for UFS device activity. This
essentially means a dedicated LED will blink upon the UFS device activity
like we have for ATA, MTD, NAND devices.

Currently, I'm not sure about the places where we have to insert this trigger.
My only choice so far is to add the trigger in `ufshcd_send_command` API,
which sends the SCSI/device management commands to the hardware.

Is there any other places where we should insert the trigger? Before that, is
this feature be acceptable?

Looking forward to the responses!

Thanks,
Mani


Re: [PATCH v4 3/6] mpt3sas: Introdude _scsih_get_shost_and_ioc.

2018-10-01 Thread Bjorn Helgaas
[+cc LKML]

On Mon, Oct 01, 2018 at 12:57:01PM +0530, Suganath Prabu Subramani wrote:
> On Thu, Sep 27, 2018 at 2:39 AM Bjorn Helgaas  wrote:
> > On Wed, Sep 26, 2018 at 09:52:36AM +0530, Suganath Prabu S wrote:
> > > The code for getting shost and IOC is redundant so
> > > moved that to function "scsih_get_shost_and_ioc".
> > > Also checks for NULL are added to IOC and shost.
> > >
> > > Signed-off-by: Suganath Prabu S 
> > > ---
> > >  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 98 
> > > ++--
> > >  1 file changed, 82 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
> > > b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> > > index 566a550..f6e92eb 100644
> > > --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> > > +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> > > @@ -9809,6 +9809,35 @@ _scsih_ir_shutdown(struct MPT3SAS_ADAPTER *ioc)
> > >  }
> > >
> > >  /**
> > > + * _scsih_get_shost_and_ioc - get shost and ioc
> > > + *   and verify whether they are NULL or not
> > > + * @pdev: PCI device struct
> > > + * @shost: address of scsi host pointer
> > > + * @ioc: address of HBA adapter pointer
> > > + *
> > > + * Return zero if *shost and *ioc are not NULL otherwise return error 
> > > number.
> > > + */
> > > +static int
> > > +_scsih_get_shost_and_ioc(struct pci_dev *pdev,
> > > + struct Scsi_Host **shost, struct MPT3SAS_ADAPTER **ioc)
> > > +{
> > > + *shost = pci_get_drvdata(pdev);
> > > + if (*shost == NULL) {
> > > + dev_err(&pdev->dev, "pdev's driver data is null\n");
> > > + return -ENXIO;
> > > + }
> > > +
> > > + *ioc = shost_priv(*shost);
> > > + if (*ioc == NULL) {
> > > + dev_err(&pdev->dev, "shost's private data is null\n");
> > > + return -ENXIO;
> >
> > I think it's better to omit NULL pointer checks like these because
> > there should not be a path where we can execute this code when these
> > pointers are NULL.
> >
> > If there *is* such a path, I think that's a serious bug and it's
> > better to oops when we try to dereference the NULL pointer.  If we
> > just return an error code, it's likely the bug will be ignored and
> > never fixed.
> >
> We have added the ioc and shost checks, because of kernel panic in
> below scenario.
> Have 3 HBA's in system and perform below operation.
> 1) Run “poweroff”.
> 2) Immediate hot unplug HBA.
> We have observed, kernel's shutdown process has removed all the 3
> HBA devices smoothly, and also user have unplugged the HBA device
> during this time. PCI subsystem's hot-plug thread is also trying to
> remove the hot plugged PCI device which is already removed/cleaned
> by the shutdown process. (Which is not expected for the already
> removed device) Due to this kernel panic is observed. And we are not
> sure whether it has to fixed from pciehp layer, so we added sanity
> checks in driver.

This is a great example.  scsih_remove() is the mpt3sas_driver.remove
method.  It sounds like it's getting called twice for the same HBA.
I think that's a PCI core defect and we should fix it there instead of
trying to work around it in every driver.

The trace below is from v4.4.55.  Can you reproduce the same problem
with a current tree, e.g., v4.19-rc?  There have been many changes
since v4.4 that may have fixed this problem.

> [ 1745.605510] BUG: unable to handle kernel NULL pointer dereference
> at 0a98
> [ 1745.606554] IP: [] scsih_remove+0x20/0x2d0 [mpt3sas]
> [ 1745.607609] PGD 0
> [ 1745.608621] Oops:  [#1] SMP
> [ 1745.622989] CPU: 0 PID: 668 Comm: kworker/0:2 Tainted: G
> O4.4.55-1.el7.elrepo.x86_64 #1
> [ 1745.624800] Hardware name: PRO-MNU65930231
> PRO-NME69559126/BRD-PRO55212588, BIOS 0.51e 05/08/2017
> [ 1745.626673] Workqueue: pciehp-3 pciehp_power_thread
> [ 1745.628566] task: 881fe50dd880 ti: 881fe88e4000 task.ti:
> 881fe88e4000
> [ 1745.630530] RIP: 0010:[]  []
> scsih_remove+0x20/0x2d0 [mpt3sas]
> [ 1745.632577] RSP: 0018:881fe88e7c98  EFLAGS: 00010292
> [ 1745.634639] RAX: 0001 RBX: 881feef5c000 RCX: 
> 
> [ 1745.636718] RDX:  RSI: 0202 RDI: 
> 881feef5c000
> [ 1745.638832] RBP: 881fe88e7cc8 R08:  R09: 
> 000180220002
> [ 1745.640972] R10: eaf4a401 R11: ea007fabd280 R12: 
> 
> [ 1745.643136] R13: a0576020 R14: 881fe9af8240 R15: 
> 
> [ 1745.645320] FS:  () GS:881ffde0()
> knlGS:
> [ 1745.647572] CS:  0010 DS:  ES:  CR0: 80050033
> [ 1745.649833] CR2: 0a98 CR3: 001fe76df000 CR4: 
> 003406f0
> [ 1745.652147] DR0:  DR1:  DR2: 
> 
> [ 1745.654476] DR3:  DR6: fffe0ff0 DR7: 
> 0400
> [ 1745.656825] Stack:
> [ 1745.659138]  881fe88e7cc8 881feef5c098 881fee

Re: [Qemu-devel] virtio-console downgrade the virtio-pci-blk performance

2018-10-01 Thread Feng Li
Hi Dave,
My comments are in-line.

Dr. David Alan Gilbert  于2018年10月1日周一 下午7:41写道:
>
> * Feng Li (lifeng1...@gmail.com) wrote:
> > Hi,
> > I found an obvious performance downgrade when virtio-console combined
> > with virtio-pci-blk.
> >
> > This phenomenon exists in nearly all Qemu versions and all Linux
> > (CentOS7, Fedora 28, Ubuntu 18.04) distros.
> >
> > This is a disk cmd:
> > -drive 
> > file=iscsi://127.0.0.1:3260/iqn.2016-02.com.test:system:fl-iscsi/1,format=raw,if=none,id=drive-virtio-disk0,cache=none,aio=native
> > -device 
> > virtio-blk-pci,scsi=off,bus=pci.0,addr=0x6,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on
> >
> > If I add "-device
> > virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x5  ", the virtio
> > disk 4k iops (randread/randwrite) would downgrade from 60k to 40k.
> >
> > In VM, if I rmmod virtio-console, the performance will back to normal.
> >
> > Any idea about this issue?
> >
> > I don't know this is a qemu issue or kernel issue.
>
> It sounds odd;  can you provide more details on:
>   a) The benchmark you're using.
I'm using fio, the config is:
[global]
ioengine=libaio
iodepth=128
runtime=120
time_based
direct=1

[randread]
stonewall
bs=4k
filename=/dev/vdb
rw=randread

>   b) the host and the guest config (number of cpus etc)
The qemu cmd is : /usr/libexec/qemu-kvm --device virtio-balloon -m 16G
--enable-kvm -cpu host -smp 8
or qemu-system-x86_64 --device virtio-balloon -m 16G --enable-kvm -cpu
host -smp 8

The result is the same.

>   c) Why are you running it with iscsi back to the same host - why not
>  just simplify the test back to a simple file?
>

Because my ISCSI target could supply a high IOPS performance.
If using a slow disk, the performance downgrade would be not so obvious.
It's easy to be seen, you could try it.


> Dave
>
> >
> > Thanks in advance.
> > --
> > Thanks and Best Regards,
> > Alex
> >
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK



-- 
Thanks and Best Regards,
Feng Li(Alex)


[PATCH] driver core: device: add BUS_ATTR_WO macro

2018-10-01 Thread Ioana Ciornei
Add BUS_ATTR_WO macro to make it easier to add attributes without
auditing the mode settings. Also, use the newly added macro where
appropriate.

Signed-off-by: Ioana Ciornei 
---
 arch/powerpc/platforms/pseries/ibmebus.c | 12 
 drivers/block/rbd.c  | 48 
 drivers/scsi/fcoe/fcoe_sysfs.c   |  4 +--
 drivers/scsi/fcoe/fcoe_transport.c   | 10 +++
 include/linux/device.h   |  2 ++
 include/scsi/libfcoe.h   |  8 +++---
 6 files changed, 43 insertions(+), 41 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/ibmebus.c 
b/arch/powerpc/platforms/pseries/ibmebus.c
index c7c1140..c75006c 100644
--- a/arch/powerpc/platforms/pseries/ibmebus.c
+++ b/arch/powerpc/platforms/pseries/ibmebus.c
@@ -261,8 +261,8 @@ static char *ibmebus_chomp(const char *in, size_t count)
return out;
 }
 
-static ssize_t ibmebus_store_probe(struct bus_type *bus,
-  const char *buf, size_t count)
+static ssize_t probe_store(struct bus_type *bus,
+  const char *buf, size_t count)
 {
struct device_node *dn = NULL;
struct device *dev;
@@ -298,10 +298,10 @@ static ssize_t ibmebus_store_probe(struct bus_type *bus,
return rc;
return count;
 }
-static BUS_ATTR(probe, 0200, NULL, ibmebus_store_probe);
+static BUS_ATTR_WO(probe);
 
-static ssize_t ibmebus_store_remove(struct bus_type *bus,
-   const char *buf, size_t count)
+static ssize_t remove_store(struct bus_type *bus,
+   const char *buf, size_t count)
 {
struct device *dev;
char *path;
@@ -325,7 +325,7 @@ static ssize_t ibmebus_store_remove(struct bus_type *bus,
return -ENODEV;
}
 }
-static BUS_ATTR(remove, 0200, NULL, ibmebus_store_remove);
+static BUS_ATTR_WO(remove);
 
 static struct attribute *ibmbus_bus_attrs[] = {
&bus_attr_probe.attr,
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 73ed5f3..703d875 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -428,14 +428,14 @@ enum rbd_dev_flags {
 module_param(single_major, bool, 0444);
 MODULE_PARM_DESC(single_major, "Use a single major number for all rbd devices 
(default: true)");
 
-static ssize_t rbd_add(struct bus_type *bus, const char *buf,
-  size_t count);
-static ssize_t rbd_remove(struct bus_type *bus, const char *buf,
- size_t count);
-static ssize_t rbd_add_single_major(struct bus_type *bus, const char *buf,
-   size_t count);
-static ssize_t rbd_remove_single_major(struct bus_type *bus, const char *buf,
-  size_t count);
+static ssize_t add_store(struct bus_type *bus, const char *buf,
+size_t count);
+static ssize_t remove_store(struct bus_type *bus, const char *buf,
+   size_t count);
+static ssize_t add_single_major_store(struct bus_type *bus, const char *buf,
+ size_t count);
+static ssize_t remove_single_major_store(struct bus_type *bus, const char *buf,
+size_t count);
 static int rbd_dev_image_probe(struct rbd_device *rbd_dev, int depth);
 
 static int rbd_dev_id_to_minor(int dev_id)
@@ -469,10 +469,10 @@ static ssize_t rbd_supported_features_show(struct 
bus_type *bus, char *buf)
return sprintf(buf, "0x%llx\n", RBD_FEATURES_SUPPORTED);
 }
 
-static BUS_ATTR(add, 0200, NULL, rbd_add);
-static BUS_ATTR(remove, 0200, NULL, rbd_remove);
-static BUS_ATTR(add_single_major, 0200, NULL, rbd_add_single_major);
-static BUS_ATTR(remove_single_major, 0200, NULL, rbd_remove_single_major);
+static BUS_ATTR_WO(add);
+static BUS_ATTR_WO(remove);
+static BUS_ATTR_WO(add_single_major);
+static BUS_ATTR_WO(remove_single_major);
 static BUS_ATTR(supported_features, 0444, rbd_supported_features_show, NULL);
 
 static struct attribute *rbd_bus_attrs[] = {
@@ -5930,9 +5930,9 @@ static ssize_t do_rbd_add(struct bus_type *bus,
goto out;
 }
 
-static ssize_t rbd_add(struct bus_type *bus,
-  const char *buf,
-  size_t count)
+static ssize_t add_store(struct bus_type *bus,
+const char *buf,
+size_t count)
 {
if (single_major)
return -EINVAL;
@@ -5940,9 +5940,9 @@ static ssize_t rbd_add(struct bus_type *bus,
return do_rbd_add(bus, buf, count);
 }
 
-static ssize_t rbd_add_single_major(struct bus_type *bus,
-   const char *buf,
-   size_t count)
+static ssize_t add_single_major_store(struct bus_type *bus,
+ const char *buf,
+ size_t count)
 {
return do_rbd_add(bus, buf, count);
 }
@@ -6046,9 +6046,9 @@ static 

Re: [PATCH] Revert "genirq/affinity: assign vectors to all possible CPUs"

2018-10-01 Thread Paul Menzel
Dear Christoph,


On 10/01/18 14:43, Paul Menzel wrote:

> On 10/01/18 14:35, Christoph Hellwig wrote:
>> On Mon, Oct 01, 2018 at 02:33:07PM +0200, Paul Menzel wrote:
>>> Date: Wed, 29 Aug 2018 17:28:45 +0200
>>>
>>> This reverts commit ef86f3a72adb8a7931f67335560740a7ad696d1d.
>>
>> This seems rather odd.  If at all you'd revert the patch adding the
>> PCI_IRQ_AFFINITY to aacraid, not core infrastructure.
> 
> Thank you for the suggestion, but that flag was added in 2016
> to the aacraid driver.
> 
>> commit 0910d8bbdd99856af1394d3d8830955abdefee4a
>> Author: Hannes Reinecke 
>> Date:   Tue Nov 8 08:11:30 2016 +0100
>>
>> scsi: aacraid: switch to pci_alloc_irq_vectors
>> 
>> Use pci_alloc_irq_vectors and drop the hand-crafted interrupt affinity
>> routines.
> 
> So what would happen, if `PCI_IRQ_AFFINITY` was removed? Will the
> system still work with the same performance?
> 
> As far as I understood, the no regression policy is there for
> exactly that reason, and it shouldn’t matter if it’s core
> infrastructure or not. As written, I have no idea, and just know
> reverting the commit in question fixes the problem here. So I’ll
> gladly test other solutions to fix this issue.

Just as another datapoint, with `PCI_IRQ_AFFINITY` removed from
`drivers/scsi/aacraid/comminit.c` in Linux 4.14.73, the driver
initializes correctly. I have no idea regarding the performance.


Kind regards,

Paul



smime.p7s
Description: S/MIME Cryptographic Signature


[PATCH RESEND] scsi: sg: Prevent potential double frees in sg driver

2018-10-01 Thread Evan Green
From: Robb Glasser 

sg_ioctl could be spammed by requests, leading to a double free in
__free_pages. This protects the entry points of sg_ioctl where the
memory could be corrupted by a double call to __free_pages if multiple
requests are happening concurrently.

Signed-off-by: Robb Glasser 
Signed-off-by: Nick Desaulniers 
Signed-off-by: Evan Green 
Cc: sta...@vger.kernel.org

---
Reposting this patch from last summer, as it looks like it fell in between
the cracks.

 drivers/scsi/sg.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 8a254bb46a9b..25579d8a16b5 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -924,8 +924,10 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned 
long arg)
return -ENXIO;
if (!access_ok(VERIFY_WRITE, p, SZ_SG_IO_HDR))
return -EFAULT;
+   mutex_lock(&sfp->parentdp->open_rel_lock);
result = sg_new_write(sfp, filp, p, SZ_SG_IO_HDR,
 1, read_only, 1, &srp);
+   mutex_unlock(&sfp->parentdp->open_rel_lock);
if (result < 0)
return result;
result = wait_event_interruptible(sfp->read_wait,
-- 
2.19.0.605.g01d371f741-goog



[PATCH] MAINTAINERS: Fix typo in cxlflash stanza

2018-10-01 Thread Matthew R. Ochs
The uapi header file listed in the cxlflash stanza has a typo.

Removed the trailing 's' from the filename.

Reported-by: Joe Perches 
Signed-off-by: Matthew R. Ochs 
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index b22e7fd..1038857 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4032,7 +4032,7 @@ M:Uma Krishnan 
 L: linux-scsi@vger.kernel.org
 S: Supported
 F: drivers/scsi/cxlflash/
-F: include/uapi/scsi/cxlflash_ioctls.h
+F: include/uapi/scsi/cxlflash_ioctl.h
 F: Documentation/powerpc/cxlflash.txt
 
 CYBERPRO FB DRIVER
-- 
2.1.0



Re: [PATCH RESEND] scsi: sg: Prevent potential double frees in sg driver

2018-10-01 Thread Nick Desaulniers
On Mon, Oct 1, 2018 at 9:16 AM Evan Green  wrote:
>
> From: Robb Glasser 
>
> sg_ioctl could be spammed by requests, leading to a double free in
> __free_pages. This protects the entry points of sg_ioctl where the
> memory could be corrupted by a double call to __free_pages if multiple
> requests are happening concurrently.
>
> Signed-off-by: Robb Glasser 
> Signed-off-by: Nick Desaulniers 
> Signed-off-by: Evan Green 
> Cc: sta...@vger.kernel.org
>
> ---
> Reposting this patch from last summer, as it looks like it fell in between
> the cracks.

Christoph, do you still feel strongly about: https://lkml.org/lkml/2017/8/5/75 ?

>
>  drivers/scsi/sg.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 8a254bb46a9b..25579d8a16b5 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -924,8 +924,10 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, 
> unsigned long arg)
> return -ENXIO;
> if (!access_ok(VERIFY_WRITE, p, SZ_SG_IO_HDR))
> return -EFAULT;
> +   mutex_lock(&sfp->parentdp->open_rel_lock);
> result = sg_new_write(sfp, filp, p, SZ_SG_IO_HDR,
>  1, read_only, 1, &srp);
> +   mutex_unlock(&sfp->parentdp->open_rel_lock);
> if (result < 0)
> return result;
> result = wait_event_interruptible(sfp->read_wait,
> --
> 2.19.0.605.g01d371f741-goog
>


-- 
Thanks,
~Nick Desaulniers


Re: [PATCH RESEND] scsi: sg: Prevent potential double frees in sg driver

2018-10-01 Thread Bart Van Assche
On Mon, 2018-10-01 at 10:12 -0700, Nick Desaulniers wrote:
> On Mon, Oct 1, 2018 at 9:16 AM Evan Green  wrote:
> > 
> > From: Robb Glasser 
> > 
> > sg_ioctl could be spammed by requests, leading to a double free in
> > __free_pages. This protects the entry points of sg_ioctl where the
> > memory could be corrupted by a double call to __free_pages if multiple
> > requests are happening concurrently.
> > 
> > Signed-off-by: Robb Glasser 
> > Signed-off-by: Nick Desaulniers 
> > Signed-off-by: Evan Green 
> > Cc: sta...@vger.kernel.org
> > 
> > ---
> > Reposting this patch from last summer, as it looks like it fell in between
> > the cracks.
> 
> Christoph, do you still feel strongly about: 
> https://lkml.org/lkml/2017/8/5/75 ?

I don't know how Christoph feels about it, but serializing all SG I/O seems
like a regression to me. If one sg command hangs I usually try to send
another sg command to the same SCSI device from another shell to get more
information about the nature of the hang. Serializing all SG I/O would make
that impossible.

Bart.


Re: [PATCH v4 1/6] mpt3sas: Introduce mpt3sas_base_pci_device_is_available

2018-10-01 Thread Bjorn Helgaas
On Mon, Oct 01, 2018 at 12:27:28PM +0530, Suganath Prabu Subramani wrote:
> On Fri, Sep 28, 2018 at 12:40 AM Lukas Wunner  wrote:
> > On Thu, Sep 27, 2018 at 01:47:46PM -0500, Bjorn Helgaas wrote:

> > > I'm not sure how mpt3sas benefits from adding
> > > mpt3sas_base_pci_device_is_available() here, other than the fact that
> > > it avoids an MMIO read to the device in most cases.  I think it would
> > > be simpler and better to make mpt3sas_base_get_iocstate() smarter
> > > about handling ~0 values from the readl().
> >
> > Avoiding an MMIO read when it's known to run into a Completion Timeout
> > buys about 17 ms.  If the function is executed many times (I don't know
> > if that's the case here, I'm referring to the general case), or if bailing
> > out of it early avoids significant amounts of further I/O, then checking
> > for disconnectedness makes sense.

I agree that if we know the device is gone, it's helpful to avoid
further I/O to it.  The misleading pattern used in this patch is:

  R1) Check for device presence
  R2) Read from device
  R3) Consume data from device

That promotes the misconception that "the device is present, so we got
valid data from it."  But in fact the device may disappear between R1
and R2, so the following pattern is better:

  A) Read from device
  B) Check data for validity, e.g., look for ~0
  C) Consume data if valid

If we're writing to the device, we don't expect any response, and it
makes sense to do:

  W1) Check for device presence
  W2) If device present, write to device

Obviously the device can disappear between W1 and W2, so this can't
eliminate *all* writes to non-existent devices, but if we want to
reduce them and we're only doing writes to the device (with no reads),
this is the best we can do.

We can learn that the device is gone in several ways: pciehp might
notice the link is down, the driver might notice a ~0 return, etc.

The driver writer knows the structure of communication with the
device, so he/she should know the appropriate places to check for
valid data from reads and where to check to minimize the number of
writes to a non-existent device.

> This function is called only during controller reset, system shutdown
> and driver unload operations.
> For this case we can remove mpt3sas_base_pci_device_is_available() check,
> since we can make the device removal from readl in
> mpt3sas_base_get_iocstate() as you suggested.

> But we need mpt3sas_base_pci_device_is_available() in other places
> like while submitting the
> IO/TMs to the HBA firmware etc where driver is not checking for the
> IOC state (through readl() operation)
> to gracefully handle the HBA hot-unplug operation.

This is the W1/W2 case above, and what you can do is constrained by
the device model, i.e., where it requires reads from the device.  I
agree you may want to check whether the device is absent to minimize
writes after a device has been removed.

I think the names "pci_device_is_present()" and
"mpt3sas_base_pci_device_is_available()" contribute to the problem
because they make promises that can't be kept -- all we can say is
that the device *was* present, but we know whether it is *still*
present.  I think it would be better if the interfaces were something
like "pci_device_is_absent()" because that gives a result we can rely
on.  If that returns true, we know the device is definitely gone.

Bjorn


Re: [PATCH] libosd: Remove ignored __weak attribute

2018-10-01 Thread Nick Desaulniers
On Sun, Sep 30, 2018 at 1:55 PM Nathan Chancellor
 wrote:
>
> Clang warns that the __weak attribute is going to be ignored on
> osd_root_object because it's not in the correct location (needs to be
> after the type).
>
> ./include/scsi/osd_types.h:31:21: warning: 'weak' attribute only applies
> to variables, functions, and classes [-Wignored-attributes]
> static const struct __weak osd_obj_id osd_root_object = {0, 0};
> ^
>
> Turns out that GCC ignores the attribute too albeit silently because
> moving the attribute after either osd_obj_id or osd_root_object like
> all other uses of __weak on variables in the kernel causes a build
> error on both GCC and Clang because static variables cannot be weak
> since weak definitions rely on not having internal linkage:
>
> ./include/scsi/osd_types.h:31:32: error: weak declaration cannot have
> internal linkage
> static const struct osd_obj_id __weak osd_root_object = {0, 0};
>^
>
> Just remove the attribute because it hasn't been correct since the
> initial addition of this file in commit de258bf5e638 ("[SCSI] libosd:
> OSDv1 Headers").
>
> Reported-by: Nick Desaulniers 
> Reviewed-by: Nick Desaulniers 
> Signed-off-by: Nathan Chancellor 
> ---
>  include/scsi/osd_types.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/scsi/osd_types.h b/include/scsi/osd_types.h
> index 48e8a165e136..6b6fdcafa6cc 100644
> --- a/include/scsi/osd_types.h
> +++ b/include/scsi/osd_types.h
> @@ -28,7 +28,7 @@ struct osd_obj_id {
> osd_id id;
>  };
>
> -static const struct __weak osd_obj_id osd_root_object = {0, 0};
> +static const struct osd_obj_id osd_root_object = {0, 0};
>
>  struct osd_attr {
> u32 attr_page;
> --
> 2.19.0
>

LGTM, thank you for sending, Nathan.
-- 
Thanks,
~Nick Desaulniers


[RESEND][PATCH] scsi: ufs: Fix hynix ufs bug with quirk on hi36xx SoC

2018-10-01 Thread John Stultz
From: Wei Li 

Hynix ufs has deviations on hi36xx platform which will result in
ufs bursts transfer failures.

To fix the problem, the Hynix device must set the register
VS_DebugSaveConfigTime to 0x10, which will set time reference
for SaveConfigTime is 250 ns. The time reference for SaveConfigTime
is 40 ns by default.

This patch is necessary to boot on HiKey960 boards that use
Hynix UFS chips.

Cc: Vinayak Holikatti 
Cc: "James E.J. Bottomley" 
Cc: "Martin K. Petersen" 
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Wei Li 
Signed-off-by: Dmitry Shmidt 
[jstultz: Forward ported from older code, slight tweak to commit message]
Signed-off-by: John Stultz 
---
 drivers/scsi/ufs/ufs-hisi.c   | 9 +
 drivers/scsi/ufs/ufs_quirks.h | 6 ++
 drivers/scsi/ufs/ufshcd.c | 2 ++
 3 files changed, 17 insertions(+)

diff --git a/drivers/scsi/ufs/ufs-hisi.c b/drivers/scsi/ufs/ufs-hisi.c
index 46df707..452e19f 100644
--- a/drivers/scsi/ufs/ufs-hisi.c
+++ b/drivers/scsi/ufs/ufs-hisi.c
@@ -20,6 +20,7 @@
 #include "unipro.h"
 #include "ufs-hisi.h"
 #include "ufshci.h"
+#include "ufs_quirks.h"
 
 static int ufs_hisi_check_hibern8(struct ufs_hba *hba)
 {
@@ -390,6 +391,14 @@ static void ufs_hisi_set_dev_cap(struct 
ufs_hisi_dev_params *hisi_param)
 
 static void ufs_hisi_pwr_change_pre_change(struct ufs_hba *hba)
 {
+   if (hba->dev_quirks & UFS_DEVICE_QUIRK_HOST_VS_DEBUGSAVECONFIGTIME) {
+   pr_info("ufs flash device must set VS_DebugSaveConfigTime 
0x10\n");
+   /* VS_DebugSaveConfigTime */
+   ufshcd_dme_set(hba, UIC_ARG_MIB(0xD0A0), 0x10);
+   /* sync length */
+   ufshcd_dme_set(hba, UIC_ARG_MIB(0x1556), 0x48);
+   }
+
/* update */
ufshcd_dme_set(hba, UIC_ARG_MIB(0x15A8), 0x1);
/* PA_TxSkip */
diff --git a/drivers/scsi/ufs/ufs_quirks.h b/drivers/scsi/ufs/ufs_quirks.h
index 71f73d1..5d2dfdb 100644
--- a/drivers/scsi/ufs/ufs_quirks.h
+++ b/drivers/scsi/ufs/ufs_quirks.h
@@ -131,4 +131,10 @@ struct ufs_dev_fix {
  */
 #define UFS_DEVICE_QUIRK_HOST_PA_SAVECONFIGTIME(1 << 8)
 
+/*
+ * Some UFS devices require VS_DebugSaveConfigTime is 0x10,
+ * enabling this quirk ensure this.
+ */
+#define UFS_DEVICE_QUIRK_HOST_VS_DEBUGSAVECONFIGTIME   (1 << 9)
+
 #endif /* UFS_QUIRKS_H_ */
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c55f38e..a67f298 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -230,6 +230,8 @@ static struct ufs_dev_fix ufs_fixups[] = {
UFS_FIX(UFS_VENDOR_SKHYNIX, UFS_ANY_MODEL, UFS_DEVICE_NO_VCCQ),
UFS_FIX(UFS_VENDOR_SKHYNIX, UFS_ANY_MODEL,
UFS_DEVICE_QUIRK_HOST_PA_SAVECONFIGTIME),
+   UFS_FIX(UFS_VENDOR_SKHYNIX, UFS_ANY_MODEL,
+   UFS_DEVICE_QUIRK_HOST_VS_DEBUGSAVECONFIGTIME),
 
END_FIX
 };
-- 
2.7.4



Re: [PATCH RESEND] scsi: sg: Prevent potential double frees in sg driver

2018-10-01 Thread Douglas Gilbert

On 2018-10-02 02:15 AM, Evan Green wrote:

From: Robb Glasser 

sg_ioctl could be spammed by requests, leading to a double free in
__free_pages. This protects the entry points of sg_ioctl where the
memory could be corrupted by a double call to __free_pages if multiple
requests are happening concurrently.


Hi,
I  don't like this patch. I would like to see the trace for the double
call to the __free_pages you are referring too. A test program that
show the fault, perhaps?

I have test code to "spam" the sg driver and have not seen a double
__free_pages that you refer to (see sg3_utils package version 1.44,
testing/sg_tst_async.cpp).

Currently I am dusting off 20 years of "laparoscopic" patches to the sg
driver that have made a bit of a mess of the naming and comments. Also
the 16 outstanding requests per file descriptor limit is being removed.
Then I want to add the SG_IOSUBMIT and SG_IORECEIVE ioctls proposed by
Linus Torvalds two week ago.

Executive summary: nak, without further information

Doug Gilbert


Signed-off-by: Robb Glasser 
Signed-off-by: Nick Desaulniers 
Signed-off-by: Evan Green 
Cc: sta...@vger.kernel.org

---
Reposting this patch from last summer, as it looks like it fell in between
the cracks.

  drivers/scsi/sg.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 8a254bb46a9b..25579d8a16b5 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -924,8 +924,10 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned 
long arg)
return -ENXIO;
if (!access_ok(VERIFY_WRITE, p, SZ_SG_IO_HDR))
return -EFAULT;
+   mutex_lock(&sfp->parentdp->open_rel_lock);
result = sg_new_write(sfp, filp, p, SZ_SG_IO_HDR,
 1, read_only, 1, &srp);
+   mutex_unlock(&sfp->parentdp->open_rel_lock);
if (result < 0)
return result;
result = wait_event_interruptible(sfp->read_wait,





Re: [PATCH 0/7] Neaten logging uses

2018-10-01 Thread Joe Perches
On Mon, 2018-09-17 at 08:01 -0700, Joe Perches wrote:
> Several defects exist in the logging uses
> 
> o Missing KERN_
> o Unnecessary KERN_ uses with panic
> o Mismatched MPT3SAS_FMT and %s: with name and __func__
> 
> Correct these defects and perhaps add some clarity to the logging.

Submitted 2 weeks ago now without comment.

ping...

> Joe Perches (7):
>   mpt3sas: Add ioc_ logging macros
>   mpt3sas: Convert uses of pr_ with MPT3SAS_FMT to ioc_
>   mpt3sas: Convert mlsleading uses of pr_ with MPT3SAS_FMT
>   mpt3sas: Convert logging uses with MPT3SAS_FMT and reply_q_name to %s:
>   mpt3sas: Remove KERN_WARNING from panic uses
>   mpt3sas: Convert logging uses with MPT3SAS_FMT without logging levels
>   mpt3sas: Remove unused macro MPT3SAS_FMT
> 
>  drivers/scsi/mpt3sas/mpt3sas_base.c | 1116 +---
>  drivers/scsi/mpt3sas/mpt3sas_base.h |9 +-
>  drivers/scsi/mpt3sas/mpt3sas_config.c   |   89 +-
>  drivers/scsi/mpt3sas/mpt3sas_ctl.c  |  493 -
>  drivers/scsi/mpt3sas/mpt3sas_scsih.c| 1487 
> ---
>  drivers/scsi/mpt3sas/mpt3sas_transport.c|  337 +++---
>  drivers/scsi/mpt3sas/mpt3sas_trigger_diag.c |  101 +-
>  drivers/scsi/mpt3sas/mpt3sas_warpdrive.c|   70 +-
>  8 files changed, 1618 insertions(+), 2084 deletions(-)
> 



Re: [PATCH] driver core: device: add BUS_ATTR_WO macro

2018-10-01 Thread Greg KH
On Mon, Oct 01, 2018 at 06:32:52PM +0300, Ioana Ciornei wrote:
> Add BUS_ATTR_WO macro to make it easier to add attributes without
> auditing the mode settings. Also, use the newly added macro where
> appropriate.
> 
> Signed-off-by: Ioana Ciornei 
> ---
>  arch/powerpc/platforms/pseries/ibmebus.c | 12 
>  drivers/block/rbd.c  | 48 
> 
>  drivers/scsi/fcoe/fcoe_sysfs.c   |  4 +--
>  drivers/scsi/fcoe/fcoe_transport.c   | 10 +++
>  include/linux/device.h   |  2 ++
>  include/scsi/libfcoe.h   |  8 +++---
>  6 files changed, 43 insertions(+), 41 deletions(-)

Nice!  This duplicates a lot of the work I did back in July but have not
pushed out very far due to the other things that ended up happening
around that time:

https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/log/?h=bus_cleanup

As the patch series seen at that link shows, you can do this in more
places than just what you did here.

Either way, you should break this up into the individual patches, like I
did or you can take my patches if you want.  Getting the BUS_ATTR_WO()
macro added is good to do now, and then you can go and hit up all of the
different subsystems that should be converted over to it.

thanks,

greg k-h


Re: [PATCH] libosd: Remove ignored __weak attribute

2018-10-01 Thread Bart Van Assche

On 9/30/18 1:54 PM, Nathan Chancellor wrote:

diff --git a/include/scsi/osd_types.h b/include/scsi/osd_types.h
index 48e8a165e136..6b6fdcafa6cc 100644
--- a/include/scsi/osd_types.h
+++ b/include/scsi/osd_types.h
@@ -28,7 +28,7 @@ struct osd_obj_id {
osd_id id;
  };
  
-static const struct __weak osd_obj_id osd_root_object = {0, 0};

+static const struct osd_obj_id osd_root_object = {0, 0};


Structure definitions should occur in .c files instead of in header 
files especially if the header file is included from multiple source 
files. Please consider moving the definition of osd_root_object into a 
.c file. Additionally, zero initializers should be left out to minimize 
the size of object files.


Boaz, the most recent osd patch that is neither trivial nor treewide 
refactoring is six years old (51976a8c85ce ("[SCSI] osd_uld: Add osdname 
& systemid sysfs at scsi_osd class"). That suggests that nobody is using 
this driver anymore. Can this driver be removed from the kernel tree?


Thanks,

Bart.


[PATCH] scsi/pmcraid.c: Use dma_pool_zalloc

2018-10-01 Thread Souptick Joarder
Replaced dma_pool_alloc + memset with dma_pool_zalloc.

Signed-off-by: Sabyasachi Gupta 
Signed-off-by: Souptick Joarder 
---
 drivers/scsi/pmcraid.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
index 4e86994..84a2734 100644
--- a/drivers/scsi/pmcraid.c
+++ b/drivers/scsi/pmcraid.c
@@ -4681,7 +4681,7 @@ static int pmcraid_allocate_control_blocks(struct 
pmcraid_instance *pinstance)
 
for (i = 0; i < PMCRAID_MAX_CMD; i++) {
pinstance->cmd_list[i]->ioa_cb =
-   dma_pool_alloc(
+   dma_pool_zalloc(
pinstance->control_pool,
GFP_KERNEL,
&(pinstance->cmd_list[i]->ioa_cb_bus_addr));
@@ -4690,8 +4690,6 @@ static int pmcraid_allocate_control_blocks(struct 
pmcraid_instance *pinstance)
pmcraid_release_control_blocks(pinstance, i);
return -ENOMEM;
}
-   memset(pinstance->cmd_list[i]->ioa_cb, 0,
-   sizeof(struct pmcraid_control_block));
}
return 0;
 }
-- 
1.9.1



[PATCH] scsi/mpt3sas: Use dma_pool_zalloc

2018-10-01 Thread Souptick Joarder
Replaced dma_pool_alloc + memset with dma_pool_zalloc.

Signed-off-by: Brajeswar Ghosh 
Signed-off-by: Souptick Joarder 
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 59d7844..d7ef671 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -4516,7 +4516,7 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
i = 0;
do {
ioc->reply_post[i].reply_post_free =
-   dma_pool_alloc(ioc->reply_post_free_dma_pool,
+   dma_pool_zalloc(ioc->reply_post_free_dma_pool,
GFP_KERNEL,
&ioc->reply_post[i].reply_post_free_dma);
if (!ioc->reply_post[i].reply_post_free) {
@@ -4525,7 +4525,6 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
ioc->name);
goto out;
}
-   memset(ioc->reply_post[i].reply_post_free, 0, sz);
dinitprintk(ioc, pr_info(MPT3SAS_FMT
"reply post free pool (0x%p): depth(%d),"
"element_size(%d), pool_size(%d kB)\n", ioc->name,
@@ -4852,14 +4851,13 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
ioc->name);
goto out;
}
-   ioc->reply_free = dma_pool_alloc(ioc->reply_free_dma_pool, GFP_KERNEL,
+   ioc->reply_free = dma_pool_zalloc(ioc->reply_free_dma_pool, GFP_KERNEL,
&ioc->reply_free_dma);
if (!ioc->reply_free) {
pr_err(MPT3SAS_FMT "reply_free pool: dma_pool_alloc failed\n",
ioc->name);
goto out;
}
-   memset(ioc->reply_free, 0, sz);
dinitprintk(ioc, pr_info(MPT3SAS_FMT "reply_free pool(0x%p): " \
"depth(%d), element_size(%d), pool_size(%d kB)\n", ioc->name,
ioc->reply_free, ioc->reply_free_queue_depth, 4, sz/1024));
-- 
1.9.1



Re: [PATCH] libosd: Remove ignored __weak attribute

2018-10-01 Thread Nathan Chancellor
On Mon, Oct 01, 2018 at 06:16:32PM -0700, Bart Van Assche wrote:
> On 9/30/18 1:54 PM, Nathan Chancellor wrote:
> > diff --git a/include/scsi/osd_types.h b/include/scsi/osd_types.h
> > index 48e8a165e136..6b6fdcafa6cc 100644
> > --- a/include/scsi/osd_types.h
> > +++ b/include/scsi/osd_types.h
> > @@ -28,7 +28,7 @@ struct osd_obj_id {
> > osd_id id;
> >   };
> > -static const struct __weak osd_obj_id osd_root_object = {0, 0};
> > +static const struct osd_obj_id osd_root_object = {0, 0};
> 

Hi Bart,

> Structure definitions should occur in .c files instead of in header files
> especially if the header file is included from multiple source files. Please
> consider moving the definition of osd_root_object into a .c file.
> Additionally, zero initializers should be left out to minimize the size of
> object files.
> 

I'm perfectly happy to make this change in a v2 if necessary.

> Boaz, the most recent osd patch that is neither trivial nor treewide
> refactoring is six years old (51976a8c85ce ("[SCSI] osd_uld: Add osdname &
> systemid sysfs at scsi_osd class"). That suggests that nobody is using this
> driver anymore. Can this driver be removed from the kernel tree?
> 

However, this is certainly a better option if possible.

> Thanks,
> 
> Bart.

Thanks for the comments!
Nathan


Re: [PATCH v4 3/6] mpt3sas: Introdude _scsih_get_shost_and_ioc.

2018-10-01 Thread Suganath Prabu Subramani
On Thu, Sep 27, 2018 at 2:39 AM Bjorn Helgaas  wrote:
>
> On Wed, Sep 26, 2018 at 09:52:36AM +0530, Suganath Prabu S wrote:
> > The code for getting shost and IOC is redundant so
> > moved that to function "scsih_get_shost_and_ioc".
> > Also checks for NULL are added to IOC and shost.
> >
> > Signed-off-by: Suganath Prabu S 
> > ---
> >  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 98 
> > ++--
> >  1 file changed, 82 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
> > b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> > index 566a550..f6e92eb 100644
> > --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> > +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> > @@ -9809,6 +9809,35 @@ _scsih_ir_shutdown(struct MPT3SAS_ADAPTER *ioc)
> >  }
> >
> >  /**
> > + * _scsih_get_shost_and_ioc - get shost and ioc
> > + *   and verify whether they are NULL or not
> > + * @pdev: PCI device struct
> > + * @shost: address of scsi host pointer
> > + * @ioc: address of HBA adapter pointer
> > + *
> > + * Return zero if *shost and *ioc are not NULL otherwise return error 
> > number.
> > + */
> > +static int
> > +_scsih_get_shost_and_ioc(struct pci_dev *pdev,
> > + struct Scsi_Host **shost, struct MPT3SAS_ADAPTER **ioc)
> > +{
> > + *shost = pci_get_drvdata(pdev);
> > + if (*shost == NULL) {
> > + dev_err(&pdev->dev, "pdev's driver data is null\n");
> > + return -ENXIO;
> > + }
> > +
> > + *ioc = shost_priv(*shost);
> > + if (*ioc == NULL) {
> > + dev_err(&pdev->dev, "shost's private data is null\n");
> > + return -ENXIO;
>
> I think it's better to omit NULL pointer checks like these because
> there should not be a path where we can execute this code when these
> pointers are NULL.
>
> If there *is* such a path, I think that's a serious bug and it's
> better to oops when we try to dereference the NULL pointer.  If we
> just return an error code, it's likely the bug will be ignored and
> never fixed.
>
We have added the ioc and shost checks, because of kernel panic in
below scenario.
Have 3 HBA's in system and perform below operation.
1) Run “poweroff”.
2) Immediate hot unplug HBA.
We have observed, kernel's shutdown process has removed all the 3 HBA
devices smoothly,
and also user have unplugged the HBA device during this time. PCI
subsystem's hot-plug thread is also trying to remove
the hot plugged PCI device which is already removed/cleaned by the
shutdown process. (Which is not expected for the
already removed device) Due to this kernel panic is observed. And we
are not sure whether it
has to fixed from pciehp layer, so we added sanity checks in driver.

[ 1745.605510] BUG: unable to handle kernel NULL pointer dereference
at 0a98
[ 1745.606554] IP: [] scsih_remove+0x20/0x2d0 [mpt3sas]
[ 1745.607609] PGD 0
[ 1745.608621] Oops:  [#1] SMP
[ 1745.622989] CPU: 0 PID: 668 Comm: kworker/0:2 Tainted: G
O4.4.55-1.el7.elrepo.x86_64 #1
[ 1745.624800] Hardware name: PRO-MNU65930231
PRO-NME69559126/BRD-PRO55212588, BIOS 0.51e 05/08/2017
[ 1745.626673] Workqueue: pciehp-3 pciehp_power_thread
[ 1745.628566] task: 881fe50dd880 ti: 881fe88e4000 task.ti:
881fe88e4000
[ 1745.630530] RIP: 0010:[]  []
scsih_remove+0x20/0x2d0 [mpt3sas]
[ 1745.632577] RSP: 0018:881fe88e7c98  EFLAGS: 00010292
[ 1745.634639] RAX: 0001 RBX: 881feef5c000 RCX: 
[ 1745.636718] RDX:  RSI: 0202 RDI: 881feef5c000
[ 1745.638832] RBP: 881fe88e7cc8 R08:  R09: 000180220002
[ 1745.640972] R10: eaf4a401 R11: ea007fabd280 R12: 
[ 1745.643136] R13: a0576020 R14: 881fe9af8240 R15: 
[ 1745.645320] FS:  () GS:881ffde0()
knlGS:
[ 1745.647572] CS:  0010 DS:  ES:  CR0: 80050033
[ 1745.649833] CR2: 0a98 CR3: 001fe76df000 CR4: 003406f0
[ 1745.652147] DR0:  DR1:  DR2: 
[ 1745.654476] DR3:  DR6: fffe0ff0 DR7: 0400
[ 1745.656825] Stack:
[ 1745.659138]  881fe88e7cc8 881feef5c098 881feef5c000
a0576020
[ 1745.661562]  881fe9af8240  881fe88e7cf0
8137f9d9
[ 1745.663990]  881feef5c098 a0576088 881feef5c000
881fe88e7d10
[ 1745.666428] Call Trace:
[ 1745.668830]  [] pci_device_remove+0x39/0xc0
[ 1745.671256]  [] __device_release_driver+0x96/0x130
[ 1745.673664]  [] device_release_driver+0x23/0x30
[ 1745.676071]  [] pci_stop_bus_device+0x8c/0xa0
[ 1745.678485]  [] pci_stop_and_remove_bus_device+0x12/0x20
[ 1745.680909]  [] pciehp_unconfigure_device+0xaa/0x1b0
[ 1745.683331]  [] pciehp_disable_slot+0x52/0xd0
[ 1745.685767]  [] pciehp_power_thread+0x8d/0xb0
[ 1745.688210]  [] process_one_work+0x14f/0x400
[ 1745.690633]  [] worker_thread+0x114/0x470
[ 174

Re: aic7xxx DMA overflow error

2018-10-01 Thread Christoph Hellwig
On Sun, Sep 30, 2018 at 07:28:50PM -0400, tedheadster wrote:
> Hannes,
>   I'm getting the following error in a custom configured 4.18 32-bit
> x86 kernel supporting PAE, with 16GiB physical memory. It loops
> infinitely on the error.
> 
> aic7xxx :00:03.0: dma_direct_map_sg: overflow
> 0x0003ff80+65536 of device mask 
> 
> I have tried enabling the following to fix this in the kernel config:
> 
> CONFIG_X86_PAE=y
> CONFIG_VMSPLIT_3G=y
> CONFIG_HIGHMEM=y
> CONFIG_HIGHMEM64G=y
> CONFIG_ZONE_DMA=y
> CONFIG_BOUNCE=y

Do you have CONFIG_SWIOTLB enabled?


Re: aic7xxx DMA overflow error

2018-10-01 Thread tedheadster
Christoph,
  I was able to bisect this to your patch "scsi: reduce use of block
bounce buffers". I am getting the error on a 32-bit Dell PowerEdge
6650. It has the aic7xxx integrated onto the motherboard.

Again, here is the error:

aic7xxx :00:03.0: dma_direct_map_sg: overflow
0x0003ff80+65536 of device mask 

I wonder if the odd 39-bit mask used in aic7xxx is part of the problem?

- Matthew


RE: [PATCH v7 3/8] uapi: ufs: Make utp_upiu_req visible to user space

2018-10-01 Thread Avri Altman
> > +
> > +/*
> > + * This file intended to be included by both kernel and user space
> > + */
> > +
> > +#define MAX_CDB_SIZE   16
> 
> Please rename this constant such that it has either "UFS" or "UTP" in its
> name. This name is too generic for a protocol-specific header file.
Done.


Thanks,
Avri

> 
> Thanks,
> 
> Bart.