RE: [PATCH 0/3] scsi: ufs-bsg: Add read descriptor
+Bean Thanks, Avri > -Original Message- > From: linux-scsi-ow...@vger.kernel.org > On Behalf Of Avri Altman > Sent: Friday, November 30, 2018 9:32 AM > To: James E.J. Bottomley ; Martin K. Petersen > ; linux-scsi@vger.kernel.org > Cc: Christoph Hellwig ; Avi Shchislowski > ; Alex Lemberg ; > Bart Van Assche ; Evan Green > ; Doug Anderson ; > Tomas Winkler ; adrian.hun...@intel.com; > Sayali Lokhande > Subject: RE: [PATCH 0/3] scsi: ufs-bsg: Add read descriptor > > A gentle ping. > > Cheers, > Avri > > > -Original Message- > > From: linux-scsi-ow...@vger.kernel.org ow...@vger.kernel.org> > > On Behalf Of Avri Altman > > Sent: Monday, November 26, 2018 11:03 AM > > To: James E.J. Bottomley ; Martin K. Petersen > > ; linux-scsi@vger.kernel.org > > Cc: Christoph Hellwig ; Bart Van Assche > > ; Avi Shchislowski > > ; Alex Lemberg ; > > Avri Altman > > Subject: [PATCH 0/3] scsi: ufs-bsg: Add read descriptor > > > > UFS Protocol Information Units (UPIU) are UFS packets that travel > > between the host and the device on the UniPro bus. Our previous series > > added the capability to send UPIUs to the ufs driver. It does not cover > > all the possible UPIU types - we are mainly focused on device > management, > > provisioning, testing and validation, so it covers UPIUs that falls in > > that box. > > > > Our intension is to publish ufs-utils soon - an open source user space > > utility that relies on that infrastructure to perform those tasks. > > This short series is adding one last functionality needed by ufs-utils > > that was somehow left behind - allowing reading descriptors as well. > > > > Avri Altman (3): > > bsg: Make job reply size different than SCSI_SENSE_BUFFERSIZE > > scsi: ufs: Allow reading descriptor via raw upiu > > scsi: ufs-bsg: Allow reading descriptors > > > > Documentation/scsi/ufs.txt | 8 > > block/bsg-lib.c| 4 ++-- > > drivers/scsi/ufs/ufs_bsg.c | 25 +++-- > > drivers/scsi/ufs/ufshcd.c | 20 ++-- > > include/linux/bsg-lib.h| 2 ++ > > 5 files changed, 41 insertions(+), 18 deletions(-) > > > > -- > > 1.9.1
Re: [PATCH v5 5/5] target: perform t10_wwn ID initialisation in target_alloc_device()
On Sat, 2018-12-01 at 15:59 +0100, Hannes Reinecke wrote:
> On 12/1/18 12:34 AM, David Disseldorp wrote:
> > Initialise the t10_wwn vendor, model and revision defaults when a
> > device is allocated instead of when it's enabled. This ensures that
> > custom vendor or model strings set prior to enablement are not later
> > overwritten with default values.
> >
> > Signed-off-by: David Disseldorp
> > ---
> > drivers/target/target_core_device.c | 34
> > +-
> > 1 file changed, 17 insertions(+), 17 deletions(-)
> > > diff --git a/drivers/target/target_core_device.c
>
> b/drivers/target/target_core_device.c
> > index 5512871f50e4..6318d59a1564 100644
> > --- a/drivers/target/target_core_device.c
> > +++ b/drivers/target/target_core_device.c
> > @@ -810,6 +810,23 @@ struct se_device *target_alloc_device(struct se_hba
> > *hba, const char *name)
> > mutex_init(_lun->lun_tg_pt_md_mutex);
> > xcopy_lun->lun_tpg = _pt_tpg;
> >
> > + /*
> > +* Preload the initial INQUIRY const values if we are doing
> > +* anything virtual (IBLOCK, FILEIO, RAMDISK), but not for TCM/pSCSI
> > +* passthrough because this is being provided by the backend LLD.
> > +*/
> > + BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
> > + BUILD_BUG_ON(sizeof(dev->t10_wwn.model) != INQUIRY_MODEL_LEN + 1);
> > + BUILD_BUG_ON(sizeof(dev->t10_wwn.revision) != INQUIRY_REVISION_LEN + 1);
> > + if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
> > + strlcpy(dev->t10_wwn.vendor, "LIO-ORG",
> > + sizeof(dev->t10_wwn.vendor));
> > + strlcpy(dev->t10_wwn.model, dev->transport->inquiry_prod,
> > + sizeof(dev->t10_wwn.model));
> > + strlcpy(dev->t10_wwn.revision, dev->transport->inquiry_rev,
> > + sizeof(dev->t10_wwn.revision));
> > + }
> > +
> > return dev;
> > }
> >
>
> This is odd. I'd rather have it consistent across backends, ie either
> move the initialisation into the backends, or provide a means to check
> if the inquiry data has already been pre-filled.
> But this check really is awkward.
Agreed. I also would like to see that that if-condition is removed ...
Thanks,
Bart.
[PATCH] hpsa: add module parameter to disable irq affinity
The PCI_IRQ_AFFINITY flag prevents customers from
changing the smp_affinity and smp_affinity_list entries.
- add a module parameter to allow this flag to be turned
off.
- to turn off PCI_IRQ_AFFINITY:
flag hpsa_disable_irq_affinity=1
Reviewed-by: David Carroll
Reviewed-by: Scott Teel
Signed-off-by: Don Brace
---
drivers/scsi/hpsa.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index c9cccf35e9d7..0aa5aa66151f 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -87,6 +87,10 @@ static int hpsa_simple_mode;
module_param(hpsa_simple_mode, int, S_IRUGO|S_IWUSR);
MODULE_PARM_DESC(hpsa_simple_mode,
"Use 'simple mode' rather than 'performant mode'");
+static bool hpsa_disable_irq_affinity;
+module_param(hpsa_disable_irq_affinity, bool, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(hpsa_disable_irq_affinity,
+ "Turn off managed irq affinity. Allows smp_affinity to be changed.");
/* define the PCI info for the cards we can control */
static const struct pci_device_id hpsa_pci_device_id[] = {
@@ -7389,7 +7393,7 @@ static void hpsa_setup_reply_map(struct ctlr_info *h)
*/
static int hpsa_interrupt_mode(struct ctlr_info *h)
{
- unsigned int flags = PCI_IRQ_LEGACY;
+ unsigned int flags;
int ret;
/* Some boards advertise MSI but don't really support it */
@@ -7400,17 +7404,20 @@ static int hpsa_interrupt_mode(struct ctlr_info *h)
case 0x40830E11:
break;
default:
+ flags = PCI_IRQ_MSIX;
+ if (!hpsa_disable_irq_affinity)
+ flags |= PCI_IRQ_AFFINITY;
ret = pci_alloc_irq_vectors(h->pdev, 1, MAX_REPLY_QUEUES,
- PCI_IRQ_MSIX | PCI_IRQ_AFFINITY);
+ flags);
if (ret > 0) {
h->msix_vectors = ret;
return 0;
}
- flags |= PCI_IRQ_MSI;
break;
}
+ flags = PCI_IRQ_LEGACY | PCI_IRQ_MSI;
ret = pci_alloc_irq_vectors(h->pdev, 1, 1, flags);
if (ret < 0)
return ret;
Re: DISABLE_CLUSTERING in scsi drivers
On Mon, 3 Dec 2018, Hannes Reinecke wrote: > As I said: I need to do PIO for the last two bytes of the data buffer. > For everything else DMA works nicely, it's just the last two bytes which > might be left over in the FIFO buffer under certain circumstances. I read the driver a few times already, thanks. > If you have an alternative suggestion without scsi_kmap_atomic_sg() I'd > be happy to convert it. I'm not trying to avoid scsi_kmap_atomic_sg(). Nevermind. -- > > Cheers, > > Hannes >
Re: DISABLE_CLUSTERING in scsi drivers
On 12/2/18 11:13 PM, Finn Thain wrote: On Sun, 2 Dec 2018, Hannes Reinecke wrote: On 12/2/18 10:21 PM, Finn Thain wrote: On Sun, 2 Dec 2018, Hannes Reinecke wrote: Well, that lone 'kmap' is due to a quirk/errata in the datasheet; essentially we have to PIO a lone byte out of the FIFO to clear it up. And this byte is technically still part of the SCSI data, so we need to stuff it onto the end of the actual data sg list. Which is what the kmap() thingie does. So it really shouldn't be affected by the clustering algorithm. Sorry, I don't follow. If it's dead code, can it be removed Oh, it's not dead code. It's required as per datasheet. If it's not, does it require DISABLE_CLUSTERING? No, not really. It just affects the very last byte of the sglist, so I can't really see how it should be affected by clustering. AIUI, the scsi_kmap_atomic_sg() call which you added to esp_scsi.c assumes that the sg list elements are page sized and page aligned. DISABLE_CLUSTERING provides that guarantee, but am53c974.c doesn't use it. Is this not a bug? What am I missing? As I said: I need to do PIO for the last two bytes of the data buffer. For everything else DMA works nicely, it's just the last two bytes which might be left over in the FIFO buffer under certain circumstances. If you have an alternative suggestion without scsi_kmap_atomic_sg() I'd be happy to convert it. Cheers, Hannes -- Dr. Hannes ReineckezSeries & Storage h...@suse.com +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)
Re: [PATCH v5 5/5] target: perform t10_wwn ID initialisation in target_alloc_device()
On Sun, 2 Dec 2018 23:22:23 +0100, David Disseldorp wrote:
> > > + if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
> > > + strlcpy(dev->t10_wwn.vendor, "LIO-ORG",
> > > + sizeof(dev->t10_wwn.vendor));
> > > + strlcpy(dev->t10_wwn.model, dev->transport->inquiry_prod,
> > > + sizeof(dev->t10_wwn.model));
> > > + strlcpy(dev->t10_wwn.revision, dev->transport->inquiry_rev,
> > > + sizeof(dev->t10_wwn.revision));
> > > + }
> > > +
> > > return dev;
> > > }
> > >
> > This is odd. I'd rather have it consistent across backends, ie either
> > move the initialisation into the backends, or provide a means to check
> > if the inquiry data has already been pre-filled.
> > But this check really is awkward.
>
> Not quite sure I follow here. I could the default setting to the
> target_backend_ops.alloc_device() code paths, but I don't think the
> duplication would make this much cleaner, if at all.
> I can look into this further if you like (target_backend_ops.inquiry_rev
> could be dropped that way),
Looking a little closer, I think we can drop the conditional completely
and set the vendor/model/rev defaults for all cases here:
- target_core_pscsi overwrites the defaults in the
pscsi_configure_device() callback.
+ the contents is then only used for configfs via
$pscsi_dev/info, $pscsi_dev/statistics/scsi_lu/vend, etc.
- target_core_user doesn't touch the defaults, nor are they used for
anything outside of configfs.
> but my preference would be to do so as a
> follow-up patch-set.
This is still my preference.
Cheers, David
