Re: [PATCH] scsi: Fix a harmless double shift bug
On 12/7/18 8:20 PM, Martin K. Petersen wrote: > > Jens, > > This went in through your tree. Can you please pick this fix up? Yep, applied, thanks Dan. -- Jens Axboe
Re: [PATCH 00/15] lpfc updates for 12.0.0.9
James, > Update lpfc to revision 12.0.0.9 > > This patch contains lpfc bug fixes Applied to 4.21/scsi-queue, thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] scsi: bnx2fc: Fix NULL dereference in error handling
Dan, > If "interface" is NULL then we can't release it and trying to will > only lead to an Oops. Applied to 4.20/scsi-fixes, thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] Revert "scsi: qla2xxx: Fix NVMe Target discovery"
Himanshu, > This reverts commit db186382af21e926e90df19499475f2552192b77. > > This commit introduced regression with FCP discovery so revert it back > to fix discovery for FCP luns Applied to 4.20/scsi-fixes. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] scsi: Fix a harmless double shift bug
Jens,
This went in through your tree. Can you please pick this fix up?
> On Thu, Nov 29, 2018 at 01:37:10PM +0300, Dan Carpenter wrote:
>> Smatch generates a warning:
>>
>> drivers/scsi/scsi_lib.c:1656 scsi_mq_done() warn: test_bit() takes a bit
>> number
>>
>> The problem is that SCMD_STATE_COMPLETE is supposed to be bit number 0
>> and not a mask like "(1 << 0)". It is used like this:
>>
>> if (test_and_set_bit(SCMD_STATE_COMPLETE, >state))
>>
>> The test_and_set_bit() has a shift built in so it's a double left shift
>> and uses bit number 1 instead of number 0. This bug is harmless because
>> it's done consistently and it doesn't clash with any other flags.
>>
>> Fixes: f1342709d18a ("scsi: Do not rely on blk-mq for double completions")
>> Signed-off-by: Dan Carpenter
>
> Nice catch, thanks for the fix.
>
> Reviewed-by: Keith Busch
Acked-by: Martin K. Petersen
--
Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 00/20] smartpqi updates
Hi Brace, In this patch series, do you have fixed and verified the timeout issue? https://patchwork.kernel.org/patch/10637797/ Thanks, - Alex Don Brace 于2018年12月8日周六 上午6:28写道： > > These patches are based on Linus's tree > > The changes are: > > - smartpqi-add-support-for-PQI-Config-Table-handshake > . add support for get/set controller features. > - smartpqi-add-retries-for-device-resets > . re-attempt device reset. > - smartpqi-add-no_write_same-for-logical-volumes > . turn off WRITE SAME for logical volumes. > - smartpqi-correct-host-serial-num-for-ssa > . update host serial number > - smartpqi-turn-off-lun-data-caching-for-ptraid > . get fresh lun list from RBODS. > - smartpqi-refactor-sending-controller-raid-requests > . condense commonly used code. > - smartpqi-add-sysfs-attributes > . add in driver attributes. > - smartpqi-add-h3c-ssid > . add support for more controllers. > - smartpqi-fix-disk-name-mount-point > . correct sysfs attribute for unique ids. > - smartpqi-wake-up-drives-after-os-resumes-from-suspend > . have OS start up disks after resume. > - smartpqi-enhance-numa-node-detection > . set pci device to correct NUMA node. > - smartpqi-add-support-for-huawei-controllers > . add support for more controllers. > - smartpqi-check-for-null-device-pointers > . wait for all outstanding I/O to complete before removing a volume > - smartpqi-allow-for-larger-raid-maps > . correct rare case for very large volume configurations. > - smartpqi-do-not-offline-disks-for-transient-did-no-connect-conditions > . remove call to scsi_device_set_state > - smartpqi-correct-volume-status > . correct rare case for volume deletion during a scan operation. > - smartpqi-correct-lun-reset-issues > . clear scsi cmd result after a reset. > - smartpqi-add-module-param-to-disable-irq-affinity > . allow some customers to change IRQ affinity. > - smartpqi-add-smp_utils-support > . add support for smp_utils. > - smartpqi-bump-driver-version > > --- > > Ajish Koshy (2): > smartpqi: add support for huawei controllers > smartpqi: allow for larger raid maps > > Dave Carroll (7): > smartpqi: add no_write_same for logical volumes > smartpqi: turn off lun data caching for ptraid > smartpqi: refactor sending controller raid requests > smartpqi: add sysfs attributes > smartpqi: wake up drives after os resumes from suspend > smartpqi: do not offline disks for transient did no connect conditions > smartpqi: correct volume status > > Don Brace (3): > smartpqi: add module param to disable irq affinity > smartpqi: add smp_utils support > smartpqi: bump driver version > > Kevin Barnett (2): > smartpqi: add support for PQI Config Table handshake > smartpqi: correct lun reset issues > > Mahesh Rajashekhara (3): > Add retries for device reset > smartpqi: correct host serial num for ssa > smartpqi: check for null device pointers > > Murthy Bhat (2): > smartpqi: add h3c ssid > smartpqi: fix disk name mount point > > Sagar Biradar (1): > smartpqi: enhance numa node detection > > > drivers/scsi/smartpqi/smartpqi.h | 144 +++ > drivers/scsi/smartpqi/smartpqi_init.c | 992 > > drivers/scsi/smartpqi/smartpqi_sas_transport.c | 164 > 3 files changed, 1131 insertions(+), 169 deletions(-) > > -- > Signature -- Thanks and Best Regards, Feng Li(Alex)
Re: [PATCH 0/2] zfcp: small bugfix on top of previous v4.21 patches
Steffen, > One new recovery fix, which is not urgent, for an old bug. It's > sufficient to apply it on top of the previously sent 23 zfcp updates > for the v4.21 merge window Applied to 4.21/scsi-queue, thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v7 0/5] target: user configurable T10 Vendor ID
David, > This patch-set allows for the modification of the T10 Vendor > Identification string returned in the SCSI INQUIRY response, via the > target/core/$backstore/$name/wwn/vendor_id ConfigFS path. Applied to 4.21/scsi-queue, thank you! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v2] qla2xxx: Split the __qla2x00_abort_all_cmds() function
Bart, > Nesting in __qla2x00_abort_all_cmds() is way too deep. Reduce the > nesting level by introducing a helper function. This patch does not > change any functionality. Applied to 4.21/scsi-queue. Thank you. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] scsi: csiostor: remove flush_scheduled_work()
Varun, > flush_scheduled_work() is not required as csio_hw_exit_workers() calls > cancel_work_sync() for hw->evtq_work. Applied to 4.21/scsi-queue, thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v2 01/23] zfcp: make DIX experimental, disabled, and independent of DIF
Steffen, > Introduce separate zfcp module parameters to individually select > support for: DIF which should work (zfcp.dif, which used to be > DIF+DIX, disabled) or DIX+DIF which can cause trouble (zfcp.dix, new, > disabled). Applied to 4.21/scsi-queue. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] scsi: ufs: Remove redundant sense size definition
Avri, > By spec, the ufs sense data is 18 bytes long. Applied to 4.21/scsi-queue, thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: BUG in copy_page_to_iter() when iscsi sets ENABLE_CLUSTERING
Note that independent of what we do in the Linux iSCSI initiator
this is a network DOS, so we'll have to fix it.
On Wed, Dec 05, 2018 at 12:09:40PM -0800, Lee Duncan wrote:
> I recently found what I believe is a bug, and I'd appreciate feedback
> on if that is correct, and if so how to proceed.
>
> BACKGROUND
>
> Recently Christoph Hellwig sent an email to driver maintainers for
> drivers that set ".use_clustering" to DISABLE_CLUSTERING in their SCSI
> Host templates, asking if the setting could be changed to
> ENABLE_CLUSTERING.
>
> As part of answering that question, I set ENABLE_CLUSTERING in
> drivers/scsi/iscsi_tcp.c and tested both the iscsi initiator and
> target.
>
> As a reminder, setting ENABLE_CLUSTERING means that adjacent bio-s can
> be merged. This can make IO faster, but it means that drivers must be
> able to deal with IOs that cross page boundaries, since bio merges can
> create such IOs.
>
> RESULTS
>
> The iscsi initiator code can handle ENABLE_CLUSTERING just fine, but
> the iscsi target code fails. It seems to assume that IOs do *NOT*
> cross a page boundary.
>
> The problem is in iscsi lib/iov_iter.c, in the functions
> copy_page_to_iter() and page_copy_sane() (see below for how to
> reproduce):
>
> >> static inline bool page_copy_sane(struct page *page, size_t offset, size_t
> >> n)
> >> {
> >> struct page *head = compound_head(page);
> >> size_t v = n + offset + page_address(page) - page_address(head);
> >>
> >> if (likely(n <= v && v <= (PAGE_SIZE << compound_order(head
> >> return true;
> >> WARN_ON(1);
> >> return false;
> >> }
> >>
> >> size_t copy_page_to_iter(struct page *page, size_t offset, size_t bytes,
> >> struct iov_iter *i)
> >> {
> >> if (unlikely(!page_copy_sane(page, offset, bytes)))
> >> return 0;
> >> if (i->type & (ITER_BVEC|ITER_KVEC)) {
> >> void *kaddr = kmap_atomic(page);
> >> size_t wanted = copy_to_iter(kaddr + offset, bytes, i);
> >> kunmap_atomic(kaddr);
> >> return wanted;
> >> } else if (unlikely(iov_iter_is_discard(i)))
> >> return bytes;
> >> else if (likely(!iov_iter_is_pipe(i)))
> >> return copy_page_to_iter_iovec(page, offset, bytes, i);
> >> else
> >> return copy_page_to_iter_pipe(page, offset, bytes, i);
> >> }
>
> Causing the following WARN_ON stack trace (repeatedly):
>
> >> ...
> >> [ 78.644559] WARNING: CPU: 0 PID: 2192 at lib/iov_iter.c:830
> >> copy_page_to_iter+0x1a6/0x2e0
> >> [ 78.644561] Modules linked in: iscsi_tcp(E) libiscsi_tcp(E) libiscsi(E)
> >> scsi_transport_iscsi(E) rfcomm(E) iscsi_target_mod(E) target_core_pscsi(E)
> >> target_core_file(E) target_core_iblock(E) target_core_user(E) uio(E)
> >> target_core_mod(E) configfs(E) af_packet(E) iscsi_ibft(E)
> >> iscsi_boot_sysfs(E) vmw_vsock_vmci_transport(E) vsock(E) bnep(E) fuse(E)
> >> crct10dif_pclmul(E) crc32_pclmul(E) ghash_clmulni_intel(E) xfs(E)
> >> aesni_intel(E) snd_ens1371(E) aes_x86_64(E) snd_ac97_codec(E)
> >> crypto_simd(E) cryptd(E) ac97_bus(E) glue_helper(E) snd_rawmidi(E)
> >> vmw_balloon(E) snd_seq_device(E) snd_pcm(E) pcspkr(E) snd_timer(E) snd(E)
> >> uvcvideo(E) btusb(E) videobuf2_vmalloc(E) btrtl(E) videobuf2_memops(E)
> >> videobuf2_v4l2(E) btbcm(E) btintel(E) videodev(E) bluetooth(E)
> >> videobuf2_common(E) vmw_vmci(E) ecdh_generic(E) rfkill(E) soundcore(E)
> >> mptctl(E) gameport(E) joydev(E) i2c_piix4(E) e1000(E) ac(E) button(E)
> >> btrfs(E) libcrc32c(E) xor(E) raid6_pq(E) hid_generic(E) usbhid(E)
> >> sr_mod(E) cdrom(E) ata_generic(E)
> >> [ 78.644583] crc32c_intel(E) serio_raw(E) mptspi(E)
> >> scsi_transport_spi(E) mptscsih(E) ata_piix(E) uhci_hcd(E) ehci_pci(E)
> >> ehci_hcd(E) vmwgfx(E) drm_kms_helper(E) syscopyarea(E) sysfillrect(E)
> >> sysimgblt(E) fb_sys_fops(E) usbcore(E) ttm(E) mptbase(E) drm(E) sg(E)
> >> dm_multipath(E) dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E)
> >> [ 78.644593] CPU: 0 PID: 2192 Comm: iscsi_trx Tainted: GE
> >> 4.20.0-rc4-1-default+ #1 openSUSE Tumbleweed (unreleased)
> >> [ 78.644593] Hardware name: VMware, Inc. VMware Virtual Platform/440BX
> >> Desktop Reference Platform, BIOS 6.00 05/19/2017
> >> [ 78.644595] RIP: 0010:copy_page_to_iter+0x1a6/0x2e0
> >> [ 78.644596] Code: 0c 24 48 98 49 89 ce 48 89 c2 49 29 c6 48 29 ca 4d 01
> >> f4 48 01 d5 48 85 c0 0f 85 71 ff ff ff 48 85 ed 0f 84 68 ff ff ff eb b2
> >> <0f> 0b 45 31 ed eb 8d bf 01 00 00 00 e8 d9 1e c5 ff 65 4c 8b 34 25
> >> [ 78.644596] RSP: 0018:a04b41eefbe0 EFLAGS: 00010206
> >> [ 78.644597] RAX: f7acc40de180 RBX: a04b41eefd80 RCX:
> >> 0028a014
> >> [ 78.644598] RDX: 2000 RSI: 1000 RDI:
> >> f7acc000
> >> [ 78.644598] RBP: f7acc40de180 R08: 8f1703786000 R09:
> >>
Re: [PATCH 01/10] gdth: refactor ioc_general
On Thu, 6 Dec 2018, Christoph Hellwig wrote:
> This function is a huge mess with duplicated error handling. Split out
> a few useful helpers and use goto labels to untangle the error handling
> and no-data ioctl handling.
>
> Signed-off-by: Christoph Hellwig
> ---
> drivers/scsi/gdth.c | 244 +++-
> 1 file changed, 126 insertions(+), 118 deletions(-)
>
> diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
> index 16709735b546..45e67d4cb3af 100644
> --- a/drivers/scsi/gdth.c
> +++ b/drivers/scsi/gdth.c
> @@ -4155,131 +4155,139 @@ static int ioc_resetdrv(void __user *arg, char
> *cmnd)
> return 0;
> }
>
> -static int ioc_general(void __user *arg, char *cmnd)
> +static void gdth_ioc_addr32(gdth_ha_str *ha, gdth_ioctl_general *gen,
> + u64 paddr)
> {
> -gdth_ioctl_general gen;
> -char *buf = NULL;
> -u64 paddr;
> -gdth_ha_str *ha;
> -int rval;
> + if (ha->cache_feat & SCATTER_GATHER) {
> + gen->command.u.cache.DestAddr = 0x;
> + gen->command.u.cache.sg_canz = 1;
> + gen->command.u.cache.sg_lst[0].sg_ptr = (u32)paddr;
> + gen->command.u.cache.sg_lst[0].sg_len = gen->data_len;
> + gen->command.u.cache.sg_lst[1].sg_len = 0;
> + } else {
> + gen->command.u.cache.DestAddr = paddr;
> + gen->command.u.cache.sg_canz = 0;
> + }
> +}
>
> -if (copy_from_user(, arg, sizeof(gdth_ioctl_general)))
> -return -EFAULT;
> -ha = gdth_find_ha(gen.ionode);
> -if (!ha)
> -return -EFAULT;
> +static void gdth_ioc_addr64(gdth_ha_str *ha, gdth_ioctl_general *gen,
> + u64 paddr)
> +{
> + if (ha->cache_feat & SCATTER_GATHER) {
> + gen->command.u.cache64.DestAddr = (u64)-1;
> + gen->command.u.cache64.sg_canz = 1;
> + gen->command.u.cache64.sg_lst[0].sg_ptr = paddr;
> + gen->command.u.cache64.sg_lst[0].sg_len = gen->data_len;
> + gen->command.u.cache64.sg_lst[1].sg_len = 0;
> + } else {
> + gen->command.u.cache64.DestAddr = paddr;
> + gen->command.u.cache64.sg_canz = 0;
> + }
> +}
>
> -if (gen.data_len > INT_MAX)
> -return -EINVAL;
> -if (gen.sense_len > INT_MAX)
> -return -EINVAL;
> -if (gen.data_len + gen.sense_len > INT_MAX)
> -return -EINVAL;
> +static void gdth_ioc_cacheservice(gdth_ha_str *ha, gdth_ioctl_general *gen,
> + u64 paddr)
> +{
> + if (ha->cache_feat & GDT_64BIT) {
> + /* copy elements from 32-bit IOCTL structure */
> + gen->command.u.cache64.BlockCnt = gen->command.u.cache.BlockCnt;
> + gen->command.u.cache64.BlockNo = gen->command.u.cache.BlockNo;
> + gen->command.u.cache64.DeviceNo = gen->command.u.cache.DeviceNo;
>
> -if (gen.data_len + gen.sense_len != 0) {
> -if (!(buf = gdth_ioctl_alloc(ha, gen.data_len + gen.sense_len,
> - FALSE, )))
> -return -EFAULT;
> -if (copy_from_user(buf, arg + sizeof(gdth_ioctl_general),
> - gen.data_len + gen.sense_len)) {
> -gdth_ioctl_free(ha, gen.data_len+gen.sense_len, buf, paddr);
> -return -EFAULT;
> -}
> + gdth_ioc_addr64(ha, gen, paddr);
> + } else {
> + gdth_ioc_addr32(ha, gen, paddr);
> + }
> +}
>
> -if (gen.command.OpCode == GDT_IOCTL) {
> -gen.command.u.ioctl.p_param = paddr;
> -} else if (gen.command.Service == CACHESERVICE) {
> -if (ha->cache_feat & GDT_64BIT) {
> -/* copy elements from 32-bit IOCTL structure */
> -gen.command.u.cache64.BlockCnt =
> gen.command.u.cache.BlockCnt;
> -gen.command.u.cache64.BlockNo = gen.command.u.cache.BlockNo;
> -gen.command.u.cache64.DeviceNo =
> gen.command.u.cache.DeviceNo;
> -/* addresses */
> -if (ha->cache_feat & SCATTER_GATHER) {
> -gen.command.u.cache64.DestAddr = (u64)-1;
> -gen.command.u.cache64.sg_canz = 1;
> -gen.command.u.cache64.sg_lst[0].sg_ptr = paddr;
> -gen.command.u.cache64.sg_lst[0].sg_len = gen.data_len;
> -gen.command.u.cache64.sg_lst[1].sg_len = 0;
> -} else {
> -gen.command.u.cache64.DestAddr = paddr;
> -gen.command.u.cache64.sg_canz = 0;
> -}
> -} else {
> -if (ha->cache_feat & SCATTER_GATHER) {
> -gen.command.u.cache.DestAddr = 0x;
> -gen.command.u.cache.sg_canz = 1;
> -gen.command.u.cache.sg_lst[0].sg_ptr = (u32)paddr;
> -gen.command.u.cache.sg_lst[0].sg_len = gen.data_len;
> -
Re: 9305-16i fault 0x5853 (regression from 4.17.2 to 4.19.2) and EEH fence errors
Would be nice to be pointed to the correct place to report major regressions, if not here. Note that the same error occurs: * on 4.18.19 * on 4.19.8-rc1 * with the latest firmware (16.00.01) * on a number of other peoples' powerpc64/power9 hardware. Note that both 4.18.X and 4.19.Y will, in addition to the above error, also occasionally hit EEH fences (on 4.18 this appears to be the predominant error, on 4.19 this is less common). [ 1711.273181] EEH: Frozen PHB#31-PE#fd detected [ 1711.273235] EEH: PE location: N/A, PHB location: N/A [ 1711.273297] CPU: 13 PID: 10993 Comm: kworker/u128:4 Tainted: G W 4.18.0-3-powerpc64le #1 Debian 4.18.20-2 [ 1711.273307] Workqueue: poll_mpt3sas0_statu _base_fault_reset_work [mpt3sas] [ 1711.273310] Call Trace: [ 1711.273314] [c00fb5623a80] [c097690c] dump_stack+0xb0/0xf4 (unreliable) [ 1711.273318] [c00fb5623ac0] [c003cad0] eeh_dev_check_failure+0x5e0/0x600 [ 1711.273321] [c00fb5623b70] [c003cb7c] eeh_check_failure+0x8c/0xd0 [ 1711.273326] [c00fb5623bb0] [c00808b6935c] mpt3sas_base_get_iocstate+0x94/0xb0 [mpt3sas] [ 1711.273330] [c00fb5623bf0] [c00808b6da10] _base_fault_reset_work+0xd8/0x310 [mpt3sas] [ 1711.273334] [c00fb5623c80] [c0129040] process_one_work+0x260/0x530 [ 1711.273336] [c00fb5623d20] [c0129398] worker_thread+0x88/0x5d0 [ 1711.273340] [c00fb5623dc0] [c0132238] kthread+0x1a8/0x1b0 [ 1711.273344] [c00fb5623e30] [c000bdd4] ret_from_kernel_thread+0x5c/0x88 [ 1711.273347] mpt3sas_cm0: SAS host is non-operational [ 1711.273690] EEH: Detected PCI bus error on PHB#31-PE#fd [ 1711.273693] EEH: This PCI device has failed 1 times in the last hour and will be permanently disabled after 5 failures. [ 1711.273694] EEH: Notify device drivers to shutdown [ 1711.273697] EEH: Beginning: 'error_detected(IO frozen)' [ 1711.273699] EEH: PE#fd (PCI 0031:01:00.0): Invoking mpt3sas->error_detected(IO frozen) [ 1711.273701] mpt3sas_cm0: PCI error: detected callback, state(2)!! [ 1711.274838] EEH: PE#fd (PCI 0031:01:00.0): mpt3sas driver reports: 'need reset' [ 1711.274839] EEH: Finished:'error_detected(IO frozen)' with aggregate recovery state:'need reset' [ 1711.274842] EEH: Collect temporary log [ 1711.274861] EEH: of node=0031:01:00.0 [ 1711.274863] EEH: PCI device/vendor: 00c41000 [ 1711.274864] EEH: PCI cmd/status register: 00100142 [ 1711.274865] EEH: PCI-E capabilities and status follow: [ 1711.274872] EEH: PCI-E 00: 0002d010 10008025 2950 00415083 [ 1711.274878] EEH: PCI-E 10: 1083 [ 1711.274879] EEH: PCI-E 20: [ 1711.274879] EEH: PCI-E AER capability register set follows: [ 1711.274886] EEH: PCI-E AER 00: 1e020001 00462031 [ 1711.274892] EEH: PCI-E AER 10: 2000 01e0 [ 1711.274897] EEH: PCI-E AER 20: [ 1711.274899] EEH: PCI-E AER 30: 00010015 [ 1711.274900] PHB4 PHB#49 Diag-data (Version: 1) [ 1711.274901] brdgCtl:0002 [ 1711.274902] RootSts:00060040 00402000 a0830008 00100107 0800 [ 1711.274902] PhbSts: 001c 001c [ 1711.274903] Lem:00011000 1000 [ 1711.274904] PhbErr: 0880 0800 214898000240 a0084000 [ 1711.274905] RxeArbErr: 0008 0008 07170176 8a82018a [ 1711.274906] PblErr: 0002 0002 [ 1711.274907] RegbErr:00400040 0040 8804 [ 1711.274908] PE[0fd] A/B: 820080230100 8a82018a [ 1711.274909] PE[100] A/B: 80003bfe 300d2593 [ 1711.274910] PE[1fd] A/B: 8000 8000 [ 1711.274910] EEH: Reset without hotplug activity [ 1716.201556] EEH: Notify device drivers the completion of reset [ 1716.201559] EEH: Beginning: 'slot_reset' [ 1716.201568] EEH: PE#fd (PCI 0031:01:00.0): Invoking mpt3sas->slot_reset() [ 1716.201569] mpt3sas_cm0: PCI error: slot reset callback!! [ 1716.201649] mpt3sas 0031:01:00.0: Using 64-bit DMA iommu bypass [ 1716.201656] mpt3sas_cm0: 64 BIT PCI BUS DMA ADDRESSING SUPPORTED, total mem (131847972 kB) [ 1716.256003] mpt3sas_cm0: CurrentHostPageSize is 0: Setting default host page size to 4k [ 1716.256024] mpt3sas_cm0: MSI-X vectors supported: 96, no of cores: 64, max_msix_vectors: -1 [ 1716.258361] mpt3sas0-msix0: PCI-MSI-X enabled: IRQ 75 [ 1716.258363] mpt3sas0-msix1: PCI-MSI-X enabled: IRQ 76 [ 1716.258364] mpt3sas0-msix2: PCI-MSI-X enabled: IRQ 77 [ 1716.258365] mpt3sas0-msix3: PCI-MSI-X enabled: IRQ 78 [ 1716.258366] mpt3sas0-msix4: PCI-MSI-X enabled: IRQ 79 [ 1716.258367] mpt3sas0-msix5: PCI-MSI-X enabled: IRQ 89 [ 1716.258368] mpt3sas0-msix6: PCI-MSI-X enabled: IRQ 90 [ 1716.258368] mpt3sas0-msix7: PCI-MSI-X enabled: IRQ 91 [ 1716.258369]
Re: [PATCH 03/10] gdth: remove gdth_{alloc,free}_ioctl
On 06/12/2018 17:50, Johannes Thumshirn wrote: > Why not calling dma_alloc_coherent() directly instead of using the > pci_alloc_consistent() wrapper? Ah should've read the whole series -- Johannes ThumshirnSUSE Labs Filesystems 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
Re: [PATCH 03/10] gdth: remove gdth_{alloc,free}_ioctl
On 06/12/2018 16:57, Christoph Hellwig wrote: > Out of the three callers once insists on the scratch buffer, and the > others are fine with a new allocation. Switch those two to juse use > pci_alloc_consistent directly, and open code the scratch buffer > allocation in the remaining one. This avoids a case where we might > be doing a memory allocation under a spinlock with irqs disabled. Why not calling dma_alloc_coherent() directly instead of using the pci_alloc_consistent() wrapper? Johannes -- Johannes ThumshirnSUSE Labs Filesystems 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
Re: [PATCH v10 4/6] scsi: ufs: Add core reset support
On 23/10/2018 06:35, Can Guo wrote:
> From: Dov Levenglick
>
> Enables core reset support. Add full initialization of the PHY and the
> controller before initializing UFS PHY and during link recovery.
>
> Signed-off-by: Dov Levenglick
> Signed-off-by: Amit Nischal
> Signed-off-by: Subhash Jadavani
> Signed-off-by: Can Guo
FWIW, this patch does not apply cleanly on top of v4.20-rc4 because
it clashes with df032bf27a414acf61c957ec2fad22a57d903b39
("scsi: ufs: Add a bsg endpoint that supports UPIUs").
Seems git's 3-way merge is smart enough to handle the conflict.
Regards.
Re: [PATCH v7 3/5] target: add device vendor_id configfs attribute
David, > Indeed, the comment should refer to page 0x83. > @Martin: all patches in this series have now been reviewed+acked. Can > you fix the above comment (s/0x80/0x83) if/when you merge, or >should I resend the series with this fixed? I'll fix it up. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v7 3/5] target: add device vendor_id configfs attribute
On Thu, 6 Dec 2018 15:25:42 +0300, Roman Bolshakov wrote: > > /* > > + * STANDARD and VPD page 0x80 T10 Vendor Identification > > Perhaps you meant 0x83 (Device Identification VPD page, T10 vendor ID > based designator). INQUIRY page 0x80 is Unit Serial Number. Indeed, the comment should refer to page 0x83. @Martin: all patches in this series have now been reviewed+acked. Can you fix the above comment (s/0x80/0x83) if/when you merge, or should I resend the series with this fixed? Cheers, David
Re: [PATCH v7 2/5] target: consistently null-terminate t10_wwn strings
On Wed, Dec 05, 2018 at 01:18:35PM +0100, David Disseldorp wrote: > In preparation for supporting user provided vendor strings, add an extra > byte to the vendor, model and revision arrays in struct t10_wwn. This > ensures that the full INQUIRY data can be carried in the arrays along > with a null-terminator. > > Change a number of array readers and writers so that they account for > explicit null-termination: > - The pscsi_set_inquiry_info() and emulate_model_alias_store() codepaths > don't currently explicitly null-terminate; fix this. > - Existing t10_wwn field dumps use for-loops which step over > null-terminators for right-padding. > + Use printf with width specifiers instead. > > Signed-off-by: David Disseldorp > --- > drivers/target/target_core_configfs.c | 16 +++ > drivers/target/target_core_device.c | 46 ++-- > drivers/target/target_core_pscsi.c| 50 > +++ > drivers/target/target_core_spc.c | 7 ++--- > drivers/target/target_core_stat.c | 32 +- > include/target/target_core_base.h | 14 +++--- > 6 files changed, 63 insertions(+), 102 deletions(-) > Reviewed-by: Roman Bolshakov Thank you, Roman Bolshakov
Re: [PATCH v7 3/5] target: add device vendor_id configfs attribute
On Wed, Dec 05, 2018 at 01:18:36PM +0100, David Disseldorp wrote: > The vendor_id attribute will allow for the modification of the T10 > Vendor Identification string returned in inquiry responses. Its value > can be viewed and modified via the ConfigFS path at: > target/core/$backstore/$name/wwn/vendor_id > > "LIO-ORG" remains the default value, which is set when the backstore > device is enabled. > > Signed-off-by: David Disseldorp > --- > drivers/target/target_core_configfs.c | 70 > +++ > 1 file changed, 70 insertions(+) > > diff --git a/drivers/target/target_core_configfs.c > b/drivers/target/target_core_configfs.c > index 8277bcf81d6e..f099c2ae451f 100644 > --- a/drivers/target/target_core_configfs.c > +++ b/drivers/target/target_core_configfs.c > @@ -1217,6 +1217,74 @@ static struct t10_wwn *to_t10_wwn(struct config_item > *item) > } > > /* > + * STANDARD and VPD page 0x80 T10 Vendor Identification Perhaps you meant 0x83 (Device Identification VPD page, T10 vendor ID based designator). INQUIRY page 0x80 is Unit Serial Number. Besides that, Reviewed-by: Roman Bolshakov Thank you, Roman
Re: [PATCH] scsi: t10-pi: Return correct ref tag when queue has no integrity profile
Hi, 在 2018/12/6 20:04, John Garry 写道: On 06/12/2018 04:17, Martin K. Petersen wrote: + Bart, Had you considered to use lower_32_bits() instead of "0x"? That would to avoid that reviewers have to count the 'f'-s to verify correctness of t10_pi_ref_tag(). I hadn't. I guess I tend to think of lower_32_bits() as something you do to pointers, not to block numbers. Xiang Chen tested the patch successfully. I'll let him provide tested-by tag. I have tested the patch with running fio on 3008 (on our ARM64 board with 4 DIF disks and 8 non-DIF disks), and now it works well. Tested-by: Xiang Chen Thanks, John .
Re: [PATCH v10 0/6] Support for Qualcomm UFS QMP PHY on SDM845
On 05/12/2018 08:01, Vivek Gautam wrote: > On Tue, Oct 23, 2018 at 10:07 AM Can Guo wrote: >> >> This patch series adds support for UFS QMP PHY on SDM845 and the >> compatible string for it. This patch series depends on the current >> proposed QMP V3 USB3 UNI PHY support for sdm845 driver [1], on >> the DT bindings for the QMP V3 USB3 PHYs based dirver [2], and also >> rebased on updated pipe_clk initialization sequence [3]. This series >> can only be merged once the dependent patches do. >> [1] >> http://lists-archives.com/linux-kernel/29071659-dt-bindings-phy-qcom-qmp-update-bindings-for-sdm845.html >> [2] >> http://lists-archives.com/linux-kernel/29071660-phy-qcom-qmp-add-qmp-v3-usb3-uni-phy-support-for-sdm845.html >> [3] https://patchwork.kernel.org/patch/10376551/ > > Besides my comment for PATCH 4/6, I have already reviewed the entire series, > and it looks good. > If adding new bindings for sdm845 needs a further review, can you separate out > just the phy patches from this series (patch 1, 2, 3 & 6), and re-send them. > We can ask Kishon if he can pull them in for this merge window. > Thanks. I'm confused. I was trying to 'git am' this series on top of v4.20 but it has been partly merged AFAICT. commit 0d58280cf1e61b06cb4d4aab672efccdc28794f6 Author: Can Guo AuthorDate: Thu Sep 20 21:27:54 2018 -0700 Commit: Kishon Vijay Abraham I CommitDate: Tue Sep 25 16:10:13 2018 +0530 commit 6b04526812ac41ba82317caa8df3549dda2cab97 Author: Can Guo AuthorDate: Thu Sep 20 21:27:55 2018 -0700 Commit: Kishon Vijay Abraham I CommitDate: Tue Sep 25 16:10:14 2018 +0530 commit cc31cdbef9b7166fe42e08267349cfbaa32696b6 Author: Can Guo AuthorDate: Thu Sep 20 21:27:56 2018 -0700 Commit: Kishon Vijay Abraham I CommitDate: Tue Sep 25 16:10:14 2018 +0530 Can't find [PATCH v10 4/6] scsi: ufs: Add core reset support [PATCH v10 5/6] scsi: ufs: Power on phy after it is initialized commit 99c7c7364b714e1de54a25c3642d991de1675e27 Author: Can Guo AuthorDate: Tue Sep 25 12:10:12 2018 +0530 Commit: Kishon Vijay Abraham I CommitDate: Tue Sep 25 16:10:14 2018 +0530 So basically, only patches 4 and 5 have not landed yet? (Are they perhaps in someone's -next tree?) Regards.
Re: [PATCH] scsi: t10-pi: Return correct ref tag when queue has no integrity profile
On 06/12/2018 04:17, Martin K. Petersen wrote: + Bart, Had you considered to use lower_32_bits() instead of "0x"? That would to avoid that reviewers have to count the 'f'-s to verify correctness of t10_pi_ref_tag(). I hadn't. I guess I tend to think of lower_32_bits() as something you do to pointers, not to block numbers. Xiang Chen tested the patch successfully. I'll let him provide tested-by tag. Thanks, John
Re: [PATCH RFC] sr: mark the device as changed when burning a CD
Dne 6.12.2018 v 11:34 Maurizio Lombardi napsal(a): > This is what I see when a cd burn operation completes: > This is the complete blktrace log: 11,034 0.81759 11653 D W 63488 (2a 00 00 03 3c 29 00 00 1f 00 ..) [wodim] 11,034 0.81759 11653 D W 63488 (2a 00 00 03 3c 29 00 00 1f 00 ..) [wodim] 11,034 0.81759 11653 D W 63488 (2a 00 00 03 3c 29 00 00 1f 00 ..) [wodim] 11,034 0.81759 11653 D W 63488 (2a 00 00 03 3c 29 00 00 1f 00 ..) [wodim] 11,038 0.049744147 11653 D W 63488 (2a 00 00 03 3c 48 00 00 1f 00 ..) [wodim] 11,038 0.049744147 11653 D W 63488 (2a 00 00 03 3c 48 00 00 1f 00 ..) [wodim] 11,038 0.049744147 11653 D W 63488 (2a 00 00 03 3c 48 00 00 1f 00 ..) [wodim] 11,038 0.049744147 11653 D W 63488 (2a 00 00 03 3c 48 00 00 1f 00 ..) [wodim] 11,03 12 0.102512363 11653 D W 63488 (2a 00 00 03 3c 67 00 00 1f 00 ..) [wodim] 11,03 12 0.102512363 11653 D W 63488 (2a 00 00 03 3c 67 00 00 1f 00 ..) [wodim] 11,03 12 0.102512363 11653 D W 63488 (2a 00 00 03 3c 67 00 00 1f 00 ..) [wodim] 11,03 12 0.102512363 11653 D W 63488 (2a 00 00 03 3c 67 00 00 1f 00 ..) [wodim] 11,03 16 0.152219178 11653 D W 63488 (2a 00 00 03 3c 86 00 00 1f 00 ..) [wodim] 11,03 16 0.152219178 11653 D W 63488 (2a 00 00 03 3c 86 00 00 1f 00 ..) [wodim] 11,03 16 0.152219178 11653 D W 63488 (2a 00 00 03 3c 86 00 00 1f 00 ..) [wodim] 11,03 16 0.152219178 11653 D W 63488 (2a 00 00 03 3c 86 00 00 1f 00 ..) [wodim] 11,03 20 0.205093330 11653 D W 63488 (2a 00 00 03 3c a5 00 00 1f 00 ..) [wodim] 11,03 20 0.205093330 11653 D W 63488 (2a 00 00 03 3c a5 00 00 1f 00 ..) [wodim] 11,03 20 0.205093330 11653 D W 63488 (2a 00 00 03 3c a5 00 00 1f 00 ..) [wodim] 11,03 20 0.205093330 11653 D W 63488 (2a 00 00 03 3c a5 00 00 1f 00 ..) [wodim] 11,03 24 0.254684616 11653 D W 63488 (2a 00 00 03 3c c4 00 00 1f 00 ..) [wodim] 11,03 24 0.254684616 11653 D W 63488 (2a 00 00 03 3c c4 00 00 1f 00 ..) [wodim] 11,03 24 0.254684616 11653 D W 63488 (2a 00 00 03 3c c4 00 00 1f 00 ..) [wodim] 11,03 24 0.254684616 11653 D W 63488 (2a 00 00 03 3c c4 00 00 1f 00 ..) [wodim] 11,03 28 0.307478799 11653 D W 63488 (2a 00 00 03 3c e3 00 00 1f 00 ..) [wodim] 11,03 28 0.307478799 11653 D W 63488 (2a 00 00 03 3c e3 00 00 1f 00 ..) [wodim] 11,03 28 0.307478799 11653 D W 63488 (2a 00 00 03 3c e3 00 00 1f 00 ..) [wodim] 11,03 28 0.307478799 11653 D W 63488 (2a 00 00 03 3c e3 00 00 1f 00 ..) [wodim] 11,03 32 0.357166787 11653 D W 63488 (2a 00 00 03 3d 02 00 00 1f 00 ..) [wodim] 11,03 32 0.357166787 11653 D W 63488 (2a 00 00 03 3d 02 00 00 1f 00 ..) [wodim] 11,03 32 0.357166787 11653 D W 63488 (2a 00 00 03 3d 02 00 00 1f 00 ..) [wodim] 11,03 32 0.357166787 11653 D W 63488 (2a 00 00 03 3d 02 00 00 1f 00 ..) [wodim] 11,03 36 0.409940696 11653 D W 63488 (2a 00 00 03 3d 21 00 00 1f 00 ..) [wodim] 11,03 36 0.409940696 11653 D W 63488 (2a 00 00 03 3d 21 00 00 1f 00 ..) [wodim] 11,03 36 0.409940696 11653 D W 63488 (2a 00 00 03 3d 21 00 00 1f 00 ..) [wodim] 11,03 36 0.409940696 11653 D W 63488 (2a 00 00 03 3d 21 00 00 1f 00 ..) [wodim] 11,03 40 0.459603865 11653 D W 63488 (2a 00 00 03 3d 40 00 00 1f 00 ..) [wodim] 11,03 40 0.459603865 11653 D W 63488 (2a 00 00 03 3d 40 00 00 1f 00 ..) [wodim] 11,03 40 0.459603865 11653 D W 63488 (2a 00 00 03 3d 40 00 00 1f 00 ..) [wodim] 11,03 40 0.459603865 11653 D W 63488 (2a 00 00 03 3d 40 00 00 1f 00 ..) [wodim] 11,03 44 0.512511601 11653 D W 63488 (2a 00 00 03 3d 5f 00 00 1f 00 ..) [wodim] 11,03 44 0.512511601 11653 D W 63488 (2a 00 00 03 3d 5f 00 00 1f 00 ..) [wodim] 11,03 44 0.512511601 11653 D W 63488 (2a 00 00 03 3d 5f 00 00 1f 00 ..) [wodim] 11,03 44 0.512511601 11653 D W 63488 (2a 00 00 03 3d 5f 00 00 1f 00 ..) [wodim] 11,03 48 0.562033003 11653 D W 63488 (2a 00 00 03 3d 7e 00 00 1f 00 ..) [wodim] 11,03 48 0.562033003 11653 D W 63488 (2a 00 00 03 3d 7e 00 00 1f 00 ..) [wodim] 11,03 48 0.562033003 11653 D W 63488 (2a 00 00 03 3d 7e 00 00 1f 00 ..) [wodim] 11,03 48 0.562033003 11653 D W 63488 (2a 00 00 03 3d 7e 00 00 1f 00 ..) [wodim] 11,03 52 0.614956620 11653 D W 63488 (2a 00 00 03 3d 9d 00 00 1f 00 ..)
Re: [PATCH RFC] sr: mark the device as changed when burning a CD
Hi Jens, Dne 20.6.2018 v 16:09 Jens Axboe napsal(a): > On 6/20/18 5:52 AM, Maurizio Lombardi wrote: >> Hi Jens, >> >> Dne 23.5.2018 v 16:42 Jens Axboe napsal(a): >>> On 5/23/18 3:19 AM, Maurizio Lombardi wrote: Dne 22.5.2018 v 16:47 Jens Axboe napsal(a): > It's been many years, but back in the day the program writing the cd > would eject the disc once done. This of course forces a reload of > the toc and clearing of the flag. What program is this? Seems like > it should probably eject when it's done. They are using wodim to burn the CDs on their servers. The problem is that they do not want the CD to be ejected because their drives lack a motorized tray, thus requiring manual intervention which they would like to avoid. >>> >>> I took a quick look at it, man that sr driver needs a bit of love :-) >>> >>> Anyway, I wonder if something like the below would work. Check for >>> a close track command in the sr completion handler, and flag the media >>> as changed if we see one. Totally untested... >>> >>> >>> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c >>> index 3f3cb72e0c0c..48f0d7a096db 100644 >>> --- a/drivers/scsi/sr.c >>> +++ b/drivers/scsi/sr.c >>> @@ -328,6 +328,9 @@ static int sr_done(struct scsi_cmnd *SCpnt) >>> scmd_printk(KERN_INFO, SCpnt, "done: %x\n", result); >>> #endif >>> >>> + if (SCpnt->cmnd[0] == GPCMD_CLOSE_TRACK) >>> + cd->device->changed = 1; >>> + >>> /* >>> * Handle MEDIUM ERRORs or VOLUME OVERFLOWs that indicate partial >>> * success. Since this is a relatively rare error condition, no >>> >> >> I just want to let you know that I tested the patch but unfortunately it >> doesn't work. >> I will try to find out what GPCMD_* commands are passed to sr_done() when >> burning a disc >> to see if there is another one which we could use. > > It's been a decade since I last messed with this or burned a cd-r, so that > would be appreciated. blktrace should be enough to tell you what commands > are being sent. > You remember this patch? The problem was that after a cd burn operation completes, you can't mount the cd unless you eject it first. I finally carried out the test you suggested by using blktrace. This is what I see when a cd burn operation completes: 11,03 32 0.357166787 11653 D W 63488 (2a 00 00 03 3d 02 00 00 1f 00 ..) [wodim] 11,03 32 0.357166787 11653 D W 63488 (2a 00 00 03 3d 02 00 00 1f 00 ..) [wodim] 11,03 32 0.357166787 11653 D W 63488 (2a 00 00 03 3d 02 00 00 1f 00 ..) [wodim] 11,03 32 0.357166787 11653 D W 63488 (2a 00 00 03 3d 02 00 00 1f 00 ..) [wodim] 11,03 80 0.922358386 11653 D W 63488 (2a 00 00 03 3e 57 00 00 1f 00 ..) [wodim] 11,03 80 0.922358386 11653 D W 63488 (2a 00 00 03 3e 57 00 00 1f 00 ..) [wodim] 11,03 80 0.922358386 11653 D W 63488 (2a 00 00 03 3e 57 00 00 1f 00 ..) [wodim] 11,03 80 0.922358386 11653 D W 63488 (2a 00 00 03 3e 57 00 00 1f 00 ..) [wodim] 11,03 112 1.332351325 11653 D W 63488 (2a 00 00 03 3f 4f 00 00 1f 00 ..) [wodim] 11,03 112 1.332351325 11653 D W 63488 (2a 00 00 03 3f 4f 00 00 1f 00 ..) [wodim] 11,03 112 1.332351325 11653 D W 63488 (2a 00 00 03 3f 4f 00 00 1f 00 ..) [wodim] 11,03 112 1.332351325 11653 D W 63488 (2a 00 00 03 3f 4f 00 00 1f 00 ..) [wodim] 11,03 152 1.791786352 11653 D W 63488 (2a 00 00 03 40 66 00 00 1f 00 ..) [wodim] 11,03 152 1.791786352 11653 D W 63488 (2a 00 00 03 40 66 00 00 1f 00 ..) [wodim] 11,03 152 1.791786352 11653 D W 63488 (2a 00 00 03 40 66 00 00 1f 00 ..) [wodim] 11,03 152 1.791786352 11653 D W 63488 (2a 00 00 03 40 66 00 00 1f 00 ..) [wodim] 11,03 196 2.357046291 11653 D W 63488 (2a 00 00 03 41 bb 00 00 1f 00 ..) [wodim] 11,03 196 2.357046291 11653 D W 63488 (2a 00 00 03 41 bb 00 00 1f 00 ..) [wodim] 11,03 196 2.357046291 11653 D W 63488 (2a 00 00 03 41 bb 00 00 1f 00 ..) [wodim] 11,03 196 2.357046291 11653 D W 63488 (2a 00 00 03 41 bb 00 00 1f 00 ..) [wodim] 11,03 304 3.600032959 11653 D N 0 (35 00 ..) [wodim] 11,03 304 3.600032959 11653 D N 0 (35 00 ..) [wodim] 11,03 304 3.600032959 11653 D N 0 (35 00 ..) [wodim] 11,03 304 3.600032959 11653 D N 0 (35 00 ..) [wodim] 11,03 33218.496219927 11653 D N 0 (35 00 ..) [wodim] 11,03 33218.496219927 11653 D N 0 (35 00 ..) [wodim] 11,03 33218.496219927 11653 D N 0 (35 00 ..) [wodim] 11,03 33218.496219927 11653 D N 0 (35 00 ..) [wodim] 11,03 35218.499114653 7433 D N 0 (00 ..) [kworker/3:2] 11,03 35218.499114653 7433 D N 0 (00 ..)
Re: [PATCH] scsi: t10-pi: Return correct ref tag when queue has no integrity profile
Bart, > Had you considered to use lower_32_bits() instead of "0x"? > That would to avoid that reviewers have to count the 'f'-s to verify > correctness of t10_pi_ref_tag(). I hadn't. I guess I tend to think of lower_32_bits() as something you do to pointers, not to block numbers. -- Martin K. Petersen Oracle Linux Engineering
Re: DISABLE_CLUSTERING in scsi drivers
On 11/21/18 1:41 AM, Christoph Hellwig wrote:
> Hi all,
>
> you in the To list maintain or wrote SCSI drivers that set the
> DISABLE_CLUSTERING flag, which basically disable merges of any
> bio segments. We already have the actual max_segment size limit
> to say which length a segment should have, independent of merged
> or originally created, so this limit generally should rarely if
> ever be required, and mostly is an old cut an paste error.
>
> Can you go over your drivers and check if it could be removed?
>
I have tested changing this to ENABLE_CLUSTERING in iscsi_tcp.c. This
code is used for both the iscsi initiator and the target.
On the initiator side, there is no problem with enabling clustering of bios.
But on the target side, the code seems to assume that IOs do not cross
page boundaries, resulting in WARN_ON messages and failed IO when
ENABLE_CLUSTERING is set:
> [ 78.644595] RIP: 0010:copy_page_to_iter+0x1a6/0x2e0
> [ 78.644596] Code: 0c 24 48 98 49 89 ce 48 89 c2 49 29 c6 48 29 ca 4d 01 f4
> 48 01 d5 48 85 c0 0f 85 71 ff ff ff 48 85 ed 0f 84 68 ff ff ff eb b2 <0f> 0b
> 45 31 ed eb 8d bf 01 00 00 00 e8 d9 1e c5 ff 65 4c 8b 34 25
> [ 78.644596] RSP: 0018:a04b41eefbe0 EFLAGS: 00010206
> [ 78.644597] RAX: f7acc40de180 RBX: a04b41eefd80 RCX:
> 0028a014
> [ 78.644598] RDX: 2000 RSI: 1000 RDI:
> f7acc000
> [ 78.644598] RBP: f7acc40de180 R08: 8f1703786000 R09:
> 2000
> [ 78.644599] R10: 02c0 R11: 8f16b3f31400 R12:
>
> [ 78.644600] R13: 2000 R14: 0008 R15:
> 3800
> [ 78.644601] FS: () GS:8f1739c0()
> knlGS:
> [ 78.644601] CS: 0010 DS: ES: CR0: 80050033
> [ 78.644602] CR2: 7f0f54772000 CR3: 00012e670004 CR4:
> 003606f0
> [ 78.644624] DR0: DR1: DR2:
>
> [ 78.644625] DR3: DR6: fffe0ff0 DR7:
> 0400
> [ 78.644625] Call Trace:
> [ 78.644631] skb_copy_datagram_iter+0x16f/0x270
> [ 78.644633] tcp_recvmsg+0x223/0xc30
> [ 78.644637] inet_recvmsg+0x4b/0xe0
> [ 78.644644] iscsit_do_rx_data.constprop.9+0x89/0x110 [iscsi_target_mod]
> [ 78.644650] rx_data+0x58/0x70 [iscsi_target_mod]
> [ 78.644654] iscsit_get_rx_pdu+0xa7b/0xd20 [iscsi_target_mod]
> [ 78.644657] ? preempt_count_sub+0x43/0x50
> [ 78.644659] ? _raw_spin_unlock_irq+0x22/0x40
> [ 78.644663] ? iscsi_target_tx_thread+0x1d0/0x1d0 [iscsi_target_mod]
> [ 78.644667] ? iscsi_target_rx_thread+0x71/0xd0 [iscsi_target_mod]
> [ 78.644670] iscsi_target_rx_thread+0x71/0xd0 [iscsi_target_mod]
> [ 78.644672] kthread+0x116/0x130
> [ 78.644673] ? kthread_create_worker_on_cpu+0x40/0x40
> [ 78.644675] ret_from_fork+0x24/0x50
> [ 78.644678] ---[ end trace a0d6d5ab0e8625ee ]---
>
The problem seems to be in iov_iter.c:copy_page_to_iter(), where it
first calls page_copy_sane(), which makes sure the copy does not cross a
page boundary:
> static inline bool page_copy_sane(struct page *page, size_t offset, size_t n)
> {
> struct page *head = compound_head(page);
> size_t v = n + offset + page_address(page) - page_address(head);
>
> if (likely(n <= v && v <= (PAGE_SIZE << compound_order(head
> return true;
> WARN_ON(1);
> return false;
> }
I will submit a separate BUG email about this to the linux-scsi and
target-devel mailing lists, since it's not clear to me how to fix this.
In the mean time, the iscsi_tcp.c file still needs DISABLE_CLUSTERING.
--
Lee
"Deviants will be sacrificed to ensure group solidarity."
Re: [PATCH] scsi: t10-pi: Return correct ref tag when queue has no integrity profile
On 12/5/18 6:04 AM, Martin K. Petersen wrote: Since the return value of this function is 'u32', can the ' & 0x' be left out? Absolutely, and I almost zapped it. However, I decided to leave it to emphasize the point that the reference tag is truncated to a 32-bit value. To me, this is more obvious than having to backtrack and spot the u32 in the function definition. I generally appreciate some sort of commentary around a return statement if the value deviates from the ordinary. The parentheses around the shift value irk me but had to leave those in place to silence gcc. Hi Martin, Had you considered to use lower_32_bits() instead of "0x"? That would to avoid that reviewers have to count the 'f'-s to verify correctness of t10_pi_ref_tag(). Thanks, Bart.
Re: [PATCH] scsi: t10-pi: Return correct ref tag when queue has no integrity profile
Hi Bart, > Since the return value of this function is 'u32', can the ' & > 0x' be left out? Absolutely, and I almost zapped it. However, I decided to leave it to emphasize the point that the reference tag is truncated to a 32-bit value. To me, this is more obvious than having to backtrack and spot the u32 in the function definition. I generally appreciate some sort of commentary around a return statement if the value deviates from the ordinary. The parentheses around the shift value irk me but had to leave those in place to silence gcc. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] scsi: t10-pi: Return correct ref tag when queue has no integrity profile
On 12/4/18 6:31 PM, Martin K. Petersen wrote:
Commit ddd0bc756983 ("block: move ref_tag calculation func to the block
layer") moved ref tag calculation from SCSI to a library function. However,
this change broke returning the correct ref tag for devices operating in
DIF mode since these do not have an associated block integrity profile.
This in turn caused read/write failures on PI-formatted disks attached to
an mpt3sas controller.
Fixes: ddd0bc756983 ("block: move ref_tag calculation func to the block layer")
Cc: sta...@vger.kernel.org # 4.19+
Reported-by: John Garry
Signed-off-by: Martin K. Petersen
---
include/linux/t10-pi.h | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/include/linux/t10-pi.h b/include/linux/t10-pi.h
index b9626aa7e90c..3e2a80cc7b56 100644
--- a/include/linux/t10-pi.h
+++ b/include/linux/t10-pi.h
@@ -39,12 +39,13 @@ struct t10_pi_tuple {
static inline u32 t10_pi_ref_tag(struct request *rq)
{
+ unsigned int shift = ilog2(queue_logical_block_size(rq->q));
+
#ifdef CONFIG_BLK_DEV_INTEGRITY
- return blk_rq_pos(rq) >>
- (rq->q->integrity.interval_exp - 9) & 0x;
-#else
- return -1U;
+ if (rq->q->integrity.interval_exp)
+ shift = rq->q->integrity.interval_exp;
#endif
+ return blk_rq_pos(rq) >> (shift - SECTOR_SHIFT) & 0x;
}
Since the return value of this function is 'u32', can the ' &
0x' be left out?
Thanks,
Bart.
Re: [PATCH v6 3/5] target: add device vendor_id configfs attribute
On Tue, 2018-12-04 at 16:26 +0300, Roman Bolshakov wrote: > wrt PATCH 5 in the series. Should we allow to set vendor_id for for > pscsi? I think we should allow that. Bart.
Re: [PATCH v6 3/5] target: add device vendor_id configfs attribute
On Tue, 4 Dec 2018 15:13:51 +0300, Roman Bolshakov wrote: > > + /* Assume ASCII encoding. Strip any newline added from userspace. */ > > + BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1); > > + strlcpy(dev->t10_wwn.vendor, strstrip(buf), > > + sizeof(dev->t10_wwn.vendor)); > > + > > Should we allow non-ASCII characters? No :) > It's okay to strip final newline > for convenience. A simple loop would ensure the rest is conformant to > SPC. EINVAL can be returned otherwise. I'll add an isascii() loop in the next round. Cheers, David
Re: [PATCH v6 3/5] target: add device vendor_id configfs attribute
On Tue, 4 Dec 2018 16:26:59 +0300, Roman Bolshakov wrote:
> wrt PATCH 5 in the series. Should we allow to set vendor_id for for
> pscsi? target_transport_configure sets t10_wwn fields for pscsi, but but
> an attempt to set vendor_id will overwrite the value recieved from
> scsi_device.
I considered adding an if (PASSTHROUGH){return -EINVAL}, but we already
allow the model/product string to be set for pscsi+tcmu backends via
emulate_model_alias, so decided against it.
> And (please correct me if I'm wrong) it's not used anywhere except
> statistics config_group. That means in the actual INQUIRY commands it
> will still return vendor_id of the underlying storage. If that's that's
> true, this means an attempt to set vendor_id will be succesful but it
> won't do what's intended for.
That's correct.
Cheers, David
Re: [PATCH v6 3/5] target: add device vendor_id configfs attribute
On Tue, Dec 04, 2018 at 03:13:51PM +0300, Roman Bolshakov wrote: > On Tue, Dec 04, 2018 at 11:12:36AM +0100, David Disseldorp wrote: > > The vendor_id attribute will allow for the modification of the T10 > > Vendor Identification string returned in inquiry responses. Its value > > can be viewed and modified via the ConfigFS path at: > > target/core/$backstore/$name/wwn/vendor_id > > > > "LIO-ORG" remains the default value, which is set when the backstore > > device is enabled. > > > > Signed-off-by: David Disseldorp > > Reviewed-by: Bryant G. Ly > > Reviewed-by: Lee Duncan > > Reviewed-by: Hannes Reinecke > > --- > > drivers/target/target_core_configfs.c | 48 > > +++ > > 1 file changed, 48 insertions(+) > > > > diff --git a/drivers/target/target_core_configfs.c > > b/drivers/target/target_core_configfs.c > > index 34872f24e8bf..67303c3f9cb4 100644 > > --- a/drivers/target/target_core_configfs.c > > +++ b/drivers/target/target_core_configfs.c > > + > > + /* Assume ASCII encoding. Strip any newline added from userspace. */ > > + BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1); > > + strlcpy(dev->t10_wwn.vendor, strstrip(buf), > > + sizeof(dev->t10_wwn.vendor)); > > + > > Should we allow non-ASCII characters? It's okay to strip final newline > for convenience. A simple loop would ensure the rest is conformant to > SPC. EINVAL can be returned otherwise. > > And for fuzzy purposes there could be a special backstore that does all > sorts of nasty things. > > According to SPC 4.3.1: > ASCII data fields shall contain only ASCII printable characters (i.e., > code values 20h to 7Eh) and may be terminated with one or more ASCII > null (00h) characters. > > 3.3.10 shall > keyword indicating a mandatory requirement > Note 1 to entry: Designers are required to implement all such > interoperability with other products that conform to this standard. > > Thank you, > Roman wrt PATCH 5 in the series. Should we allow to set vendor_id for for pscsi? target_transport_configure sets t10_wwn fields for pscsi, but but an attempt to set vendor_id will overwrite the value recieved from scsi_device. And (please correct me if I'm wrong) it's not used anywhere except statistics config_group. That means in the actual INQUIRY commands it will still return vendor_id of the underlying storage. If that's that's true, this means an attempt to set vendor_id will be succesful but it won't do what's intended for. Perhaps -ENOSYS or an error code of your preference could be returned if device has TRANSPORT_FLAG_PASSTHROUGH. Roman
Re: [PATCH v6 5/5] target: perform t10_wwn ID initialisation in target_alloc_device()
On Tue, Dec 04, 2018 at 11:12:38AM +0100, 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. > > The TRANSPORT_FLAG_PASSTHROUGH conditional can be dropped for the > following reasons: > - 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. > > Signed-off-by: David Disseldorp > --- > drivers/target/target_core_device.c | 27 ++- > 1 file changed, 10 insertions(+), 17 deletions(-) > Reviewed-by: Roman Bolshakov Thanks, Roman
Re: [PATCH v6 4/5] target: remove hardcoded T10 Vendor ID in INQUIRY response
On Tue, Dec 04, 2018 at 11:12:37AM +0100, David Disseldorp wrote: > Use the value stored in t10_wwn.vendor, which defaults to "LIO-ORG", but > can be reconfigured via the vendor_id ConfigFS attribute. > > Signed-off-by: David Disseldorp > Reviewed-by: Bryant G. Ly > Reviewed-by: Lee Duncan > Reviewed-by: Hannes Reinecke > --- > drivers/target/target_core_spc.c | 8 +--- > 1 file changed, 5 insertions(+), 3 deletions(-) > Reviewed-by: Roman Bolshakov Thank you, Roman
Re: [PATCH v6 3/5] target: add device vendor_id configfs attribute
On Tue, Dec 04, 2018 at 11:12:36AM +0100, David Disseldorp wrote: > The vendor_id attribute will allow for the modification of the T10 > Vendor Identification string returned in inquiry responses. Its value > can be viewed and modified via the ConfigFS path at: > target/core/$backstore/$name/wwn/vendor_id > > "LIO-ORG" remains the default value, which is set when the backstore > device is enabled. > > Signed-off-by: David Disseldorp > Reviewed-by: Bryant G. Ly > Reviewed-by: Lee Duncan > Reviewed-by: Hannes Reinecke > --- > drivers/target/target_core_configfs.c | 48 > +++ > 1 file changed, 48 insertions(+) > > diff --git a/drivers/target/target_core_configfs.c > b/drivers/target/target_core_configfs.c > index 34872f24e8bf..67303c3f9cb4 100644 > --- a/drivers/target/target_core_configfs.c > +++ b/drivers/target/target_core_configfs.c > + > + /* Assume ASCII encoding. Strip any newline added from userspace. */ > + BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1); > + strlcpy(dev->t10_wwn.vendor, strstrip(buf), > + sizeof(dev->t10_wwn.vendor)); > + Should we allow non-ASCII characters? It's okay to strip final newline for convenience. A simple loop would ensure the rest is conformant to SPC. EINVAL can be returned otherwise. And for fuzzy purposes there could be a special backstore that does all sorts of nasty things. According to SPC 4.3.1: ASCII data fields shall contain only ASCII printable characters (i.e., code values 20h to 7Eh) and may be terminated with one or more ASCII null (00h) characters. 3.3.10 shall keyword indicating a mandatory requirement Note 1 to entry: Designers are required to implement all such interoperability with other products that conform to this standard. Thank you, Roman
Re: [PATCH v5 2/5] target: consistently null-terminate t10_wwn strings
On Sat, Dec 01, 2018 at 12:34:20AM +0100, David Disseldorp wrote:
> In preparation for supporting user provided vendor strings, add an extra
> byte to the vendor, model and revision arrays in struct t10_wwn. This
> ensures that the full INQUIRY data can be carried in the arrays along
> with a null-terminator.
>
> Change a number of array readers and writers so that they account for
> explicit null-termination:
> - The pscsi_set_inquiry_info() and emulate_model_alias_store() codepaths
> don't currently explicitly null-terminate; fix this.
> - Existing t10_wwn field dumps use for-loops which step over
> null-terminators for right-padding.
> + Use printf with width specifiers instead.
>
> Signed-off-by: David Disseldorp
> ---
> drivers/target/target_core_configfs.c | 14 +++---
> drivers/target/target_core_device.c | 49
> ---
> drivers/target/target_core_pscsi.c| 18 -
> drivers/target/target_core_spc.c | 7 ++---
> drivers/target/target_core_stat.c | 32 +--
> include/target/target_core_base.h | 14 +++---
> 6 files changed, 61 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/target/target_core_configfs.c
> b/drivers/target/target_core_configfs.c
> index f6b1549f4142..34872f24e8bf 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -613,12 +613,17 @@ static void dev_set_t10_wwn_model_alias(struct
> se_device *dev)
> const char *configname;
>
> configname = config_item_name(>dev_group.cg_item);
> - if (strlen(configname) >= 16) {
> + if (strlen(configname) >= INQUIRY_MODEL_LEN) {
> pr_warn("dev[%p]: Backstore name '%s' is too long for "
> "INQUIRY_MODEL, truncating to 16 bytes\n", dev,
The warning (which I understand predates your patch) is misleading, it
should mention truncation to 15 instead of 16 bytes and your comment
just below explains this.
> configname);
> }
> - snprintf(>t10_wwn.model[0], 16, "%s", configname);
> + /*
> + * XXX We can't use sizeof(dev->t10_wwn.model) (INQUIRY_MODEL_LEN + 1)
> + * here without potentially breaking existing setups, so continue to
> + * truncate one byte shorter than what can be carried in INQUIRY.
> + */
> + strlcpy(dev->t10_wwn.model, configname, INQUIRY_MODEL_LEN);
> }
>
> diff --git a/drivers/target/target_core_device.c
> b/drivers/target/target_core_device.c
> index 47b5ef153135..5512871f50e4 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -1008,12 +989,16 @@ int target_configure_device(struct se_device *dev)
>* 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);
I'm sorry I'm missing something. Why BUILD_BUG_ON is added in many
places?
> diff --git a/drivers/target/target_core_pscsi.c
> b/drivers/target/target_core_pscsi.c
> index 47d76c862014..1002829f2038 100644
> --- a/drivers/target/target_core_pscsi.c
> +++ b/drivers/target/target_core_pscsi.c
> @@ -190,9 +190,15 @@ pscsi_set_inquiry_info(struct scsi_device *sdev, struct
> t10_wwn *wwn)
> /*
>* Use sdev->inquiry from drivers/scsi/scsi_scan.c:scsi_alloc_sdev()
>*/
> - memcpy(>vendor[0], [8], sizeof(wwn->vendor));
> - memcpy(>model[0], [16], sizeof(wwn->model));
> - memcpy(>revision[0], [32], sizeof(wwn->revision));
> + BUILD_BUG_ON(sizeof(wwn->vendor) != INQUIRY_VENDOR_LEN + 1);
> + snprintf(wwn->vendor, sizeof(wwn->vendor),
> + "%." __stringify(INQUIRY_VENDOR_LEN) "s", [8]);
> + BUILD_BUG_ON(sizeof(wwn->model) != INQUIRY_MODEL_LEN + 1);
> + snprintf(wwn->model, sizeof(wwn->model),
> + "%." __stringify(INQUIRY_MODEL_LEN) "s", [16]);
> + BUILD_BUG_ON(sizeof(wwn->revision) != INQUIRY_REVISION_LEN + 1);
> + snprintf(wwn->revision, sizeof(wwn->revision),
> + "%." __stringify(INQUIRY_REVISION_LEN) "s", [32]);
> }
>
The parts of the sdev->inquiry have been already right-padded with
spaces by scsi_sanitize_inquiry_string in scsi_probe_lun. Thus, it's
enough to replace sizeof with the new length definitions. Also, it's
possible to use sdev->model,vendor,rev pointers like in
pscsi_show_configfs_dev_params instead of explicit offsets [8],
[16], [32].
> static int
> @@ -826,21 +832,21 @@ static ssize_t pscsi_show_configfs_dev_params(struct
> se_device *dev, char *b)
> if (sd) {
> bl += sprintf(b + bl, "");
> bl += sprintf(b + bl, "Vendor: ");
> - for (i = 0; i < 8; i++) {
> +
Re: [PATCH v5 1/5] target: use consistent left-aligned ASCII INQUIRY data
On Sat, Dec 01, 2018 at 12:34:19AM +0100, David Disseldorp wrote: > spc5r17.pdf specifies: > 4.3.1 ASCII data field requirements > ASCII data fields shall contain only ASCII printable characters (i.e., > code values 20h to 7Eh) and may be terminated with one or more ASCII > null (00h) characters. > ASCII data fields described as being left-aligned shall have any > unused bytes at the end of the field (i.e., highest offset) and the > unused bytes shall be filled with ASCII space characters (20h). > > LIO currently space-pads the T10 VENDOR IDENTIFICATION and PRODUCT > IDENTIFICATION fields in the standard INQUIRY data. However, the > PRODUCT REVISION LEVEL field in the standard INQUIRY data as well as the > T10 VENDOR IDENTIFICATION field in the INQUIRY Device Identification VPD > Page are zero-terminated/zero-padded. > > Fix this inconsistency by using space-padding for all of the above > fields. > > Signed-off-by: David Disseldorp > Reviewed-by: Christoph Hellwig > Reviewed-by: Bryant G. Ly > Reviewed-by: Lee Duncan > --- > drivers/target/target_core_spc.c | 17 - > 1 file changed, 12 insertions(+), 5 deletions(-) > Reviewed-by: Roman Bolshakov Best regards, Roman
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.
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
Re: [PATCH v5 5/5] target: perform t10_wwn ID initialisation in target_alloc_device()
Hi Hannes,
Thanks for the feedback...
On Sat, 1 Dec 2018 15:59:25 +0100, Hannes Reinecke wrote:
> On 12/1/18 12:34 AM, David Disseldorp wrote:
...
> > @@ -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.
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), but my preference would be to do so as a
follow-up patch-set.
Cheers, David
Re: DISABLE_CLUSTERING in scsi drivers
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? BTW, Documentation/block/biodoc.txt suggests a bounce buffer as an alternative. There are some situations when pages from high memory may need to be kmapped, even if bounce buffers are not necessary. For example a device may need to abort DMA operations and revert to PIO for the transfer, in which case a virtual mapping of the page is required. For SCSI it is also done in some scenarios where the low level driver cannot be trusted to handle a single sg entry correctly. The driver is expected to perform the kmaps as needed on such occasions as appropriate. A driver could also use the blk_queue_bounce() routine on its own to bounce highmem i/o to low memory for specific requests if so desired. -- > Cheers, > > Hannes > >
Re: DISABLE_CLUSTERING in scsi drivers
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. Cheers, Hannes
Re: DISABLE_CLUSTERING in scsi drivers
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? If it's not, does it require DISABLE_CLUSTERING? -- > Cheers, > > Hannes >
Re: DISABLE_CLUSTERING in scsi drivers
On 11/26/18 10:46 AM, Finn Thain wrote: On Mon, 26 Nov 2018, Christoph Hellwig wrote: On Thu, Nov 22, 2018 at 09:02:13AM +1100, Finn Thain wrote: you in the To list maintain or wrote SCSI drivers that set the DISABLE_CLUSTERING flag, which basically disable merges of any bio segments. We already have the actual max_segment size limit to say which length a segment should have, independent of merged or originally created, so this limit generally should rarely if ever be required, and mostly is an old cut an paste error. Are you referring to blk_queue_max_segment_size(q, dma_get_max_seg_size(dev)); in drivers/scsi/scsi_lib.c? Is the segment size limitation of the DMA controller the only reason to want DISABLE_CLUSTERING? DISABLE_CLUSTERING mixes up two not really related things: 1) limit the size of each segment to a single page size 2) limit each segment to not actually span a page boundary. Both could be valid limit for DMA engines, but also might be particularly relevant for pio, if you e.g. kmap each page of a scatterlist do do pio you'd want to see both limits. I looked through all the drivers based on esp_scsi.c and NCR5380.c which use DISABLE_CLUSTERING. There is one driver that uses DMA and sometimes kmap too, which is am53c974.c. It defaults to ENABLE_CLUSTERING, which would seem to be a bug if kmap isn't compatible with that (Hannes?). 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. Cheers, Hannes
Re: [PATCH v5 5/5] target: perform t10_wwn ID initialisation in target_alloc_device()
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.
Cheers,
Hannes
Re: [PATCH v5 4/5] target: remove hardcoded T10 Vendor ID in INQUIRY response
On 12/1/18 12:34 AM, David Disseldorp wrote: Use the value stored in t10_wwn.vendor, which defaults to "LIO-ORG", but can be reconfigured via the vendor_id ConfigFS attribute. Signed-off-by: David Disseldorp Reviewed-by: Bryant G. Ly Reviewed-by: Lee Duncan --- drivers/target/target_core_spc.c | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) I would have merged it with the previous patch, but anyway: Reviewed-by: Hannes Reinecke Cheers, Hannes
Re: [PATCH v5 3/5] target: add device vendor_id configfs attribute
On 12/1/18 12:34 AM, David Disseldorp wrote: The vendor_id attribute will allow for the modification of the T10 Vendor Identification string returned in inquiry responses. Its value can be viewed and modified via the ConfigFS path at: target/core/$backstore/$name/wwn/vendor_id "LIO-ORG" remains the default value, which is set when the backstore device is enabled. Signed-off-by: David Disseldorp Reviewed-by: Bryant G. Ly Reviewed-by: Lee Duncan --- drivers/target/target_core_configfs.c | 48 +++ 1 file changed, 48 insertions(+) I have been wanting to do this for a long time now ... Reviewed-by: Hannes Reinecke Cheers, Hannes
Re: [PATCH v5 2/5] target: consistently null-terminate t10_wwn strings
On 12/1/18 12:34 AM, David Disseldorp wrote: In preparation for supporting user provided vendor strings, add an extra byte to the vendor, model and revision arrays in struct t10_wwn. This ensures that the full INQUIRY data can be carried in the arrays along with a null-terminator. Change a number of array readers and writers so that they account for explicit null-termination: - The pscsi_set_inquiry_info() and emulate_model_alias_store() codepaths don't currently explicitly null-terminate; fix this. - Existing t10_wwn field dumps use for-loops which step over null-terminators for right-padding. + Use printf with width specifiers instead. Signed-off-by: David Disseldorp --- drivers/target/target_core_configfs.c | 14 +++--- drivers/target/target_core_device.c | 49 --- drivers/target/target_core_pscsi.c| 18 - drivers/target/target_core_spc.c | 7 ++--- drivers/target/target_core_stat.c | 32 +-- include/target/target_core_base.h | 14 +++--- 6 files changed, 61 insertions(+), 73 deletions(-) Reviewed-by: Hannes Reinecke Cheers, Hannes
Re: [PATCH v5 1/5] target: use consistent left-aligned ASCII INQUIRY data
On 12/1/18 12:34 AM, David Disseldorp wrote: spc5r17.pdf specifies: 4.3.1 ASCII data field requirements ASCII data fields shall contain only ASCII printable characters (i.e., code values 20h to 7Eh) and may be terminated with one or more ASCII null (00h) characters. ASCII data fields described as being left-aligned shall have any unused bytes at the end of the field (i.e., highest offset) and the unused bytes shall be filled with ASCII space characters (20h). LIO currently space-pads the T10 VENDOR IDENTIFICATION and PRODUCT IDENTIFICATION fields in the standard INQUIRY data. However, the PRODUCT REVISION LEVEL field in the standard INQUIRY data as well as the T10 VENDOR IDENTIFICATION field in the INQUIRY Device Identification VPD Page are zero-terminated/zero-padded. Fix this inconsistency by using space-padding for all of the above fields. Signed-off-by: David Disseldorp Reviewed-by: Christoph Hellwig Reviewed-by: Bryant G. Ly Reviewed-by: Lee Duncan --- drivers/target/target_core_spc.c | 17 - 1 file changed, 12 insertions(+), 5 deletions(-) Reviewed-by: Hannes Reinecke Cheers, Hannes
Re: [PATCH v4 1/7] target: use consistent left-aligned ASCII INQUIRY data
On 11/30/18 4:44 AM, David Disseldorp wrote: > On Wed, 28 Nov 2018 17:23:07 -0800, Lee Duncan wrote: > >>> +* unused bytes at the end of the field (i.e., highest offset) and the >>> +* unused bytes shall be filled with ASCII space characters (20h). >>> +*/ >>> + memset([8], 0x20, 8 + 16 + 4); >> >> I dislike that you are using 0x20 here (and below) instead of ' '. > > Given that this patch already has a couple of reviewed-bys, I'd prefer > to avoid respinning it for this. Besides, I think the comment above > makes it pretty clear. > > Thanks for your feedback. > > Cheers, David > Also, the "0x20" is in use widely, I see, so your patch is fine with me. Please feel free to also add my: Reviewed-by: Lee Duncan If you wish.
Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response
On Fri, 2018-11-30 at 15:41 +0100, David Disseldorp wrote: > On Fri, 30 Nov 2018 14:17:49 +0100, David Disseldorp wrote: > > > > Where is the code that initializes dev->t10_wwn.vendor to "LIO-ORG"? Did I > > > perhaps overlook something? > > > > This is done in target_configure_device() . > > Hmm, continuing to do it there may cause a bit of confusion if vendor_id > is set before the backstore is enabled, which would cause it to be > overwritten. That said, we already have similarly strange behaviour for > emulate_model_alias / product. E.g.: > > rapido1:/# cd /sys/kernel/config/target/ > rapido1:target# mkdir -p core/fileio_1/testing > rapido1:target# echo -n "fd_dev_name=/asdf,fd_dev_size=4096" > > core/fileio_1/testing/control > rapido1:target# echo -n "testing1" > core/fileio_1/testing/wwn/vendor_id > rapido1:target# cat core/fileio_1/testing/wwn/vendor_id > testing1 > rapido1:target# echo 1 > ./core/fileio_1/testing/attrib/emulate_model_alias > rapido1:target# cat ./core/fileio_1/testing/statistics/scsi_lu/prod > testing > rapido1:target# echo 1 > core/fileio_1/testing/enable > rapido1:target# cat core/fileio_1/testing/wwn/vendor_id > LIO-ORG > rapido1:target# cat ./core/fileio_1/testing/statistics/scsi_lu/prod > FILEIO > rapido1:target# cat ./core/fileio_1/testing/attrib/emulate_model_alias > 1 > > Not sure how best to handle this... > 1. move vendor/model/rev initialization into target_alloc_device() > 2. move vendor (only) initialization into target_alloc_device() > 3. fail attempts to set emulate_model_alias or vendor_id before the >backstore has been enabled > 4. leave as-is > > (1) would IMO be the most straightforward, but it's a slight change to > the existing (IMO broken) emulate_model_alias user interface. I'm in favor of moving some of the target_configure_device() code into the target_alloc_device() function. Today target_configure_device() overwrites the vendor, model and revision string and is called when "1" is written into the "enable" attribute. Overwriting these attributes when enabling a backstore seems wrong to me. Thanks, Bart.
Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response
On Fri, 30 Nov 2018 14:17:49 +0100, David Disseldorp wrote: > > Where is the code that initializes dev->t10_wwn.vendor to "LIO-ORG"? Did I > > perhaps overlook something? > > This is done in target_configure_device() . Hmm, continuing to do it there may cause a bit of confusion if vendor_id is set before the backstore is enabled, which would cause it to be overwritten. That said, we already have similarly strange behaviour for emulate_model_alias / product. E.g.: rapido1:/# cd /sys/kernel/config/target/ rapido1:target# mkdir -p core/fileio_1/testing rapido1:target# echo -n "fd_dev_name=/asdf,fd_dev_size=4096" > core/fileio_1/testing/control rapido1:target# echo -n "testing1" > core/fileio_1/testing/wwn/vendor_id rapido1:target# cat core/fileio_1/testing/wwn/vendor_id testing1 rapido1:target# echo 1 > ./core/fileio_1/testing/attrib/emulate_model_alias rapido1:target# cat ./core/fileio_1/testing/statistics/scsi_lu/prod testing rapido1:target# echo 1 > core/fileio_1/testing/enable rapido1:target# cat core/fileio_1/testing/wwn/vendor_id LIO-ORG rapido1:target# cat ./core/fileio_1/testing/statistics/scsi_lu/prod FILEIO rapido1:target# cat ./core/fileio_1/testing/attrib/emulate_model_alias 1 Not sure how best to handle this... 1. move vendor/model/rev initialization into target_alloc_device() 2. move vendor (only) initialization into target_alloc_device() 3. fail attempts to set emulate_model_alias or vendor_id before the backstore has been enabled 4. leave as-is (1) would IMO be the most straightforward, but it's a slight change to the existing (IMO broken) emulate_model_alias user interface. Cheers, David
Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response
On Thu, 29 Nov 2018 08:35:26 -0800, Bart Van Assche wrote: > Where is the code that initializes dev->t10_wwn.vendor to "LIO-ORG"? Did I > perhaps overlook something? This is done in target_configure_device() . > Additionally, why are you using strnlen() for > a string of which it is guaranteed that its length is less than or equal to > the second strnlen() argument? Belt and braces :) Actually, I think it's a little more readable this way. Cheers, David
RE: [PATCH 0/3] scsi: ufs-bsg: Add read descriptor
A gentle ping. Cheers, Avri > -Original Message- > From: linux-scsi-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 v4 5/7] target: add device vendor_id configfs attribute
On Thu, 29 Nov 2018 08:32:47 -0800, Bart Van Assche wrote:
> > + unsigned char buf[INQUIRY_VENDOR_LEN + 1];
> > +
> > + if (strlen(page) > INQUIRY_VENDOR_LEN) {
> > + pr_err("Emulated T10 Vendor Identification exceeds"
> > + " INQUIRY_VENDOR_LEN: " __stringify(INQUIRY_VENDOR_LEN)
> > + "\n");
> > + return -EOVERFLOW;
> > + }
>
> Does this force users to use "echo -n" to configure vendor IDs that are
> INQUIRY_VENDOR_LEN bytes long?
It does. As mentioned in v3, this follows the convention used in
target_wwn_vpd_unit_serial_store(). I don't feel too strongly about it,
but it does make buf allocation a little less error prone IMO.
Cheers, David
Re: [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model
On Thu, 2018-11-29 at 21:31 +0100, David Disseldorp wrote: > On Thu, 29 Nov 2018 08:24:38 -0800, Bart Van Assche wrote: > > On Thu, 2018-11-29 at 02:01 +0100, David Disseldorp wrote: > > > [ ... ] > > Additionally, have you considered to use strlcpy() > > instead of snprintf()? > > Happy to change the logic below over if you find it easier to follow. It would make the code shorter without hurting readability, so I think it would be better to use strlcpy(). But it's not that important to me. Bart.
Re: [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model
On Thu, 29 Nov 2018 08:24:38 -0800, Bart Van Assche wrote:
> On Thu, 2018-11-29 at 02:01 +0100, David Disseldorp wrote:
> > diff --git a/drivers/target/target_core_configfs.c
> > b/drivers/target/target_core_configfs.c
> > index f6b1549f4142..9f49b1afd685 100644
> > --- a/drivers/target/target_core_configfs.c
> > +++ b/drivers/target/target_core_configfs.c
> > @@ -613,12 +613,12 @@ static void dev_set_t10_wwn_model_alias(struct
> > se_device *dev)
> > const char *configname;
> >
> > configname = config_item_name(>dev_group.cg_item);
> > - if (strlen(configname) >= 16) {
> > + if (strlen(configname) >= INQUIRY_MODEL_LEN) {
> > pr_warn("dev[%p]: Backstore name '%s' is too long for "
> > "INQUIRY_MODEL, truncating to 16 bytes\n", dev,
> > configname);
> > }
> > - snprintf(>t10_wwn.model[0], 16, "%s", configname);
> > + snprintf(>t10_wwn.model[0], INQUIRY_MODEL_LEN, "%s", configname);
>
> Both the old and the new statement truncate inquiry strings that are 16 bytes
> long, which is a bug.
As mentioned in the changelog, I don't think we can fix this without
potentially breaking existing deployments - e.g. a "fourfourfourfour"
backstore name with emulate_model_alias=1 would change inquiry product
ID from "fourfourfourfou" to "fourfourfourfour" following kernel
upgrade.
> Additionally, have you considered to use strlcpy()
> instead of snprintf()?
Happy to change the logic below over if you find it easier to follow.
Cheers, David
Re: [PATCH v4 7/7] target: use printf width specifier for t10_wwn field dumps
On Thu, 2018-11-29 at 02:01 +0100, David Disseldorp wrote: > The existing for loops step over null-terminators for right-padding. > Padding can be achieved in a much simpler way using printf width > specifiers. How about squashing patches 2, 3, 4 and 7 into a single patch? I think that would make the entire patch series easier to review. Thanks, Bart.
Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response
On Thu, 2018-11-29 at 02:01 +0100, David Disseldorp wrote:
> Use the value stored in t10_wwn.vendor, which defaults to "LIO-ORG", but
> can be reconfigured via the vendor_id ConfigFS attribute.
>
> Signed-off-by: David Disseldorp
> ---
> drivers/target/target_core_spc.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/target/target_core_spc.c
> b/drivers/target/target_core_spc.c
> index 8ffe712cb44d..4503f3336bc2 100644
> --- a/drivers/target/target_core_spc.c
> +++ b/drivers/target/target_core_spc.c
> @@ -115,7 +115,8 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char
> *buf)
>*/
> memset([8], 0x20,
> INQUIRY_VENDOR_LEN + INQUIRY_MODEL_LEN + INQUIRY_REVISION_LEN);
> - memcpy([8], "LIO-ORG", sizeof("LIO-ORG") - 1);
> + memcpy([8], dev->t10_wwn.vendor,
> +strnlen(dev->t10_wwn.vendor, INQUIRY_VENDOR_LEN));
> memcpy([16], dev->t10_wwn.model,
> strnlen(dev->t10_wwn.model, INQUIRY_MODEL_LEN));
> memcpy([32], dev->t10_wwn.revision,
> @@ -258,8 +259,9 @@ spc_emulate_evpd_83(struct se_cmd *cmd, unsigned char
> *buf)
> buf[off+1] = 0x1; /* T10 Vendor ID */
> buf[off+2] = 0x0;
> /* left align Vendor ID and pad with spaces */
> - memset([off+4], 0x20, 8);
> - memcpy([off+4], "LIO-ORG", sizeof("LIO-ORG") - 1);
> + memset([off+4], 0x20, INQUIRY_VENDOR_LEN);
> + memcpy([off+4], dev->t10_wwn.vendor,
> +strnlen(dev->t10_wwn.vendor, INQUIRY_VENDOR_LEN));
> /* Extra Byte for NULL Terminator */
> id_len++;
> /* Identifier Length */
Where is the code that initializes dev->t10_wwn.vendor to "LIO-ORG"? Did I
perhaps overlook something? Additionally, why are you using strnlen() for
a string of which it is guaranteed that its length is less than or equal to
the second strnlen() argument?
Thanks,
Bart.
Re: [PATCH v4 5/7] target: add device vendor_id configfs attribute
On Thu, 2018-11-29 at 02:01 +0100, David Disseldorp wrote:
> +static ssize_t target_wwn_vendor_id_show(struct config_item *item,
> + char *page)
> +{
> + return sprintf(page, "%s\n", _t10_wwn(item)->vendor[0]);
> +}
The "&" and "[0]" are superfluous in the above sprintf() statement.
> +static ssize_t target_wwn_vendor_id_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct t10_wwn *t10_wwn = to_t10_wwn(item);
> + struct se_device *dev = t10_wwn->t10_dev;
> + unsigned char buf[INQUIRY_VENDOR_LEN + 1];
> +
> + if (strlen(page) > INQUIRY_VENDOR_LEN) {
> + pr_err("Emulated T10 Vendor Identification exceeds"
> + " INQUIRY_VENDOR_LEN: " __stringify(INQUIRY_VENDOR_LEN)
> + "\n");
> + return -EOVERFLOW;
> + }
Does this force users to use "echo -n" to configure vendor IDs that are
INQUIRY_VENDOR_LEN bytes long?
Bart.
Re: [PATCH v4 4/7] target: consistently null-terminate t10_wwn.revision
On Thu, 2018-11-29 at 02:01 +0100, David Disseldorp wrote: > strncpy(>t10_wwn.revision[0], > - dev->transport->inquiry_rev, 4); > + dev->transport->inquiry_rev, INQUIRY_REVISION_LEN); > + dev->t10_wwn.revision[INQUIRY_REVISION_LEN] = '\0'; Can the above two statements be changed into a single strlcpy() call? > - memcpy(>revision[0], [32], sizeof(wwn->revision)); > + memcpy(>revision[0], [32], INQUIRY_REVISION_LEN); > + wwn->revision[INQUIRY_REVISION_LEN] = '\0'; Have you considered to use snprintf(..., "%.*s", ...) instead? Thanks, Bart.
Re: [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model
On Thu, 2018-11-29 at 02:01 +0100, David Disseldorp wrote:
> diff --git a/drivers/target/target_core_configfs.c
> b/drivers/target/target_core_configfs.c
> index f6b1549f4142..9f49b1afd685 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -613,12 +613,12 @@ static void dev_set_t10_wwn_model_alias(struct
> se_device *dev)
> const char *configname;
>
> configname = config_item_name(>dev_group.cg_item);
> - if (strlen(configname) >= 16) {
> + if (strlen(configname) >= INQUIRY_MODEL_LEN) {
> pr_warn("dev[%p]: Backstore name '%s' is too long for "
> "INQUIRY_MODEL, truncating to 16 bytes\n", dev,
> configname);
> }
> - snprintf(>t10_wwn.model[0], 16, "%s", configname);
> + snprintf(>t10_wwn.model[0], INQUIRY_MODEL_LEN, "%s", configname);
Both the old and the new statement truncate inquiry strings that are 16 bytes
long, which is a bug. Additionally, have you considered to use strlcpy()
instead of snprintf()?
> strncpy(>t10_wwn.model[0],
> - dev->transport->inquiry_prod, 16);
> + dev->transport->inquiry_prod, INQUIRY_MODEL_LEN);
> + dev->t10_wwn.model[INQUIRY_MODEL_LEN] = '\0';
Have you considered to use strlcpy() instead of strncpy() followed by explicit
'\0'-termination?
> strncpy(>t10_wwn.model[0],
> - dev->transport->inquiry_prod, 16);
> + dev->transport->inquiry_prod, INQUIRY_MODEL_LEN);
> + dev->t10_wwn.model[INQUIRY_MODEL_LEN] = '\0';
Same question here: have you considered to use strlcpy() instead of strncpy()
followed by explicit '\0'-termination?
> - memcpy(>model[0], [16], sizeof(wwn->model));
> + BUILD_BUG_ON(sizeof(wwn->model) != INQUIRY_MODEL_LEN + 1);
> + memcpy(>model[0], [16], INQUIRY_MODEL_LEN);
> + wwn->model[INQUIRY_MODEL_LEN] = '\0';
Can the memcpy() and '\0'-termination be changed into an snprintf(..., "%.*s",
...)
call?
Thanks,
Bart.
Re: [PATCH v4 2/7] target: consistently null-terminate t10_wwn.vendor
On Thu, 2018-11-29 at 02:01 +0100, David Disseldorp wrote: > - strncpy(>t10_wwn.vendor[0], "LIO-ORG", 8); > + strncpy(>t10_wwn.vendor[0], "LIO-ORG", INQUIRY_VENDOR_LEN); > + dev->t10_wwn.vendor[INQUIRY_VENDOR_LEN] = '\0'; This looks weird to me. Have you considered to use strlcpy() instead of strncpy() and explicit '\0'-termination? > strncpy(>t10_wwn.model[0], > dev->transport->inquiry_prod, 16); > strncpy(>t10_wwn.revision[0], > diff --git a/drivers/target/target_core_pscsi.c > b/drivers/target/target_core_pscsi.c > index 47d76c862014..ee65b5bb674c 100644 > --- a/drivers/target/target_core_pscsi.c > +++ b/drivers/target/target_core_pscsi.c > @@ -190,7 +190,9 @@ pscsi_set_inquiry_info(struct scsi_device *sdev, struct > t10_wwn *wwn) > /* >* Use sdev->inquiry from drivers/scsi/scsi_scan.c:scsi_alloc_sdev() >*/ > - memcpy(>vendor[0], [8], sizeof(wwn->vendor)); > + BUILD_BUG_ON(sizeof(wwn->vendor) != INQUIRY_VENDOR_LEN + 1); > + memcpy(>vendor[0], [8], INQUIRY_VENDOR_LEN); > + wwn->vendor[INQUIRY_VENDOR_LEN] = '\0'; Have you considered to use snprintf(..., "%.*s", ...) instead of memcpy() followed by explicit '\0'-termination? I think that would result in code that is easier to read. strlcpy() can't be used here because it is not guaranteed that the input buffer is '\0'-terminated. Thanks, Bart.
Re: [PATCH] scsi: Fix a harmless double shift bug
On Thu, Nov 29, 2018 at 01:37:10PM +0300, Dan Carpenter wrote:
> Smatch generates a warning:
>
> drivers/scsi/scsi_lib.c:1656 scsi_mq_done() warn: test_bit() takes a bit
> number
>
> The problem is that SCMD_STATE_COMPLETE is supposed to be bit number 0
> and not a mask like "(1 << 0)". It is used like this:
>
> if (test_and_set_bit(SCMD_STATE_COMPLETE, >state))
>
> The test_and_set_bit() has a shift built in so it's a double left shift
> and uses bit number 1 instead of number 0. This bug is harmless because
> it's done consistently and it doesn't clash with any other flags.
>
> Fixes: f1342709d18a ("scsi: Do not rely on blk-mq for double completions")
> Signed-off-by: Dan Carpenter
Nice catch, thanks for the fix.
Reviewed-by: Keith Busch
Re: [PATCH 0/2] Two refactoring patches for the qla2xxx driver
Bart, > The two patches in this series make the qla2xxx driver source code > easier to read without changing the driver functionality. Please > consider these patches for kernel v4.21. I applied patch #1. #2 had conflicts, please rebase. Thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v2 0/9] qedi bug fixes
Nilesh, > Please consider below patch set for next 'scsi-fixes' submission. Some of these smelled more like features than bug fixes. So I applied the series to 4.21/scsi-queue. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 01/23] zfcp: make DIX experimental, disabled, and independent of DIF
Steffen, As I said, I don't have a problem with having module parameters. > There's one more important thing that has performance impact: We need to > pack payload and protection data into the same queue of limited > length. So for the worst case with DIX, we have to use half the size for > sg_tablesize to get the other half for sg_prot_tablesize. Interesting. With the exception of NVMe over PCIe, it's kind of unusual for modern controllers to have real limits in this area. > This limits the maximum I/O request size and thus throughput. Using > read_verify and write_generate does not change the tablesizes, as zfcp > would still announce support for DIF and DIX. With the new zfcp.dif=1 > and zfcp.dix=0, we can use the full sg_tablesize for payload data and > sg_prot_tablesize=0. (The DIF "overhead" on the fibre still exists of > course.) > > Are there other ways for accomplishing this which I'm not aware of? You can set your shost->sg_prot_tablesize. There are pathological corner cases like dd to the raw block device where you'll suffer if there is not a 1:1 mapping between data and protection segments. But for regular I/O where the protection is generated over a whole bio at a time, you can get away with something smaller. Anyway. I don't have any problems with you making DIX experimental for zfcp. Just want to make sure it's done for the right reasons (i.e. not problems in SCSI or the block layer). -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v4 7/7] target: use printf width specifier for t10_wwn field dumps
On 11/28/18 5:01 PM, David Disseldorp wrote:
> From: Bart Van Assche
>
> The existing for loops step over null-terminators for right-padding.
> Padding can be achieved in a much simpler way using printf width
> specifiers.
>
> Signed-off-by: David Disseldorp
> ---
> drivers/target/target_core_device.c | 35 ---
> drivers/target/target_core_stat.c | 32 +++-
> 2 files changed, 15 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/target/target_core_device.c
> b/drivers/target/target_core_device.c
> index b3d0bd1ab09f..7f2d307e411b 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -720,36 +720,17 @@ void core_dev_free_initiator_node_lun_acl(
> static void scsi_dump_inquiry(struct se_device *dev)
> {
> struct t10_wwn *wwn = >t10_wwn;
> - char buf[INQUIRY_MODEL_LEN + 1];
> - int i, device_type;
> + int device_type = dev->transport->get_device_type(dev);
> +
> /*
>* Print Linux/SCSI style INQUIRY formatting to the kernel ring buffer
>*/
> - for (i = 0; i < INQUIRY_VENDOR_LEN; i++)
> - if (wwn->vendor[i] >= 0x20)
> - buf[i] = wwn->vendor[i];
> - else
> - buf[i] = ' ';
> - buf[i] = '\0';
> - pr_debug(" Vendor: %s\n", buf);
> -
> - for (i = 0; i < INQUIRY_MODEL_LEN; i++)
> - if (wwn->model[i] >= 0x20)
> - buf[i] = wwn->model[i];
> - else
> - buf[i] = ' ';
> - buf[i] = '\0';
> - pr_debug(" Model: %s\n", buf);
> -
> - for (i = 0; i < INQUIRY_REVISION_LEN; i++)
> - if (wwn->revision[i] >= 0x20)
> - buf[i] = wwn->revision[i];
> - else
> - buf[i] = ' ';
> - buf[i] = '\0';
> - pr_debug(" Revision: %s\n", buf);
> -
> - device_type = dev->transport->get_device_type(dev);
> + pr_debug(" Vendor: %-" __stringify(INQUIRY_VENDOR_LEN) "s\n",
> + wwn->vendor);
> + pr_debug(" Model: %-" __stringify(INQUIRY_MODEL_LEN) "s\n",
> + wwn->model);
> + pr_debug(" Revision: %-" __stringify(INQUIRY_REVISION_LEN) "s\n",
> + wwn->revision);
> pr_debug(" Type: %s ", scsi_device_type(device_type));
> }
>
> diff --git a/drivers/target/target_core_stat.c
> b/drivers/target/target_core_stat.c
> index e437ba494865..87fd2b11fe3b 100644
> --- a/drivers/target/target_core_stat.c
> +++ b/drivers/target/target_core_stat.c
> @@ -246,43 +246,25 @@ static ssize_t target_stat_lu_lu_name_show(struct
> config_item *item, char *page)
> static ssize_t target_stat_lu_vend_show(struct config_item *item, char *page)
> {
> struct se_device *dev = to_stat_lu_dev(item);
> - int i;
> - char str[INQUIRY_VENDOR_LEN+1];
>
> - /* scsiLuVendorId */
> - for (i = 0; i < INQUIRY_VENDOR_LEN; i++)
> - str[i] = ISPRINT(dev->t10_wwn.vendor[i]) ?
> - dev->t10_wwn.vendor[i] : ' ';
> - str[i] = '\0';
> - return snprintf(page, PAGE_SIZE, "%s\n", str);
> + return snprintf(page, PAGE_SIZE, "%-" __stringify(INQUIRY_VENDOR_LEN)
> + "s\n", dev->t10_wwn.vendor);
> }
>
> static ssize_t target_stat_lu_prod_show(struct config_item *item, char *page)
> {
> struct se_device *dev = to_stat_lu_dev(item);
> - int i;
> - char str[INQUIRY_MODEL_LEN+1];
>
> - /* scsiLuProductId */
> - for (i = 0; i < INQUIRY_MODEL_LEN; i++)
> - str[i] = ISPRINT(dev->t10_wwn.model[i]) ?
> - dev->t10_wwn.model[i] : ' ';
> - str[i] = '\0';
> - return snprintf(page, PAGE_SIZE, "%s\n", str);
> + return snprintf(page, PAGE_SIZE, "%-" __stringify(INQUIRY_MODEL_LEN)
> + "s\n", dev->t10_wwn.model);
> }
>
> static ssize_t target_stat_lu_rev_show(struct config_item *item, char *page)
> {
> struct se_device *dev = to_stat_lu_dev(item);
> - int i;
> - char str[INQUIRY_REVISION_LEN+1];
> -
> - /* scsiLuRevisionId */
> - for (i = 0; i < INQUIRY_REVISION_LEN; i++)
> - str[i] = ISPRINT(dev->t10_wwn.revision[i]) ?
> - dev->t10_wwn.revision[i] : ' ';
> - str[i] = '\0';
> - return snprintf(page, PAGE_SIZE, "%s\n", str);
> +
> + return snprintf(page, PAGE_SIZE, "%-" __stringify(INQUIRY_REVISION_LEN)
> + "s\n", dev->t10_wwn.revision);
> }
>
> static ssize_t target_stat_lu_dev_type_show(struct config_item *item, char
> *page)
>
Reviewed-by: Lee Duncan
Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response
On 11/28/18 5:01 PM, David Disseldorp wrote:
> Use the value stored in t10_wwn.vendor, which defaults to "LIO-ORG", but
> can be reconfigured via the vendor_id ConfigFS attribute.
>
> Signed-off-by: David Disseldorp
> ---
> drivers/target/target_core_spc.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/target/target_core_spc.c
> b/drivers/target/target_core_spc.c
> index 8ffe712cb44d..4503f3336bc2 100644
> --- a/drivers/target/target_core_spc.c
> +++ b/drivers/target/target_core_spc.c
> @@ -115,7 +115,8 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char
> *buf)
>*/
> memset([8], 0x20,
> INQUIRY_VENDOR_LEN + INQUIRY_MODEL_LEN + INQUIRY_REVISION_LEN);
> - memcpy([8], "LIO-ORG", sizeof("LIO-ORG") - 1);
> + memcpy([8], dev->t10_wwn.vendor,
> +strnlen(dev->t10_wwn.vendor, INQUIRY_VENDOR_LEN));
> memcpy([16], dev->t10_wwn.model,
> strnlen(dev->t10_wwn.model, INQUIRY_MODEL_LEN));
> memcpy([32], dev->t10_wwn.revision,
> @@ -258,8 +259,9 @@ spc_emulate_evpd_83(struct se_cmd *cmd, unsigned char
> *buf)
> buf[off+1] = 0x1; /* T10 Vendor ID */
> buf[off+2] = 0x0;
> /* left align Vendor ID and pad with spaces */
> - memset([off+4], 0x20, 8);
> - memcpy([off+4], "LIO-ORG", sizeof("LIO-ORG") - 1);
> + memset([off+4], 0x20, INQUIRY_VENDOR_LEN);
> + memcpy([off+4], dev->t10_wwn.vendor,
> +strnlen(dev->t10_wwn.vendor, INQUIRY_VENDOR_LEN));
> /* Extra Byte for NULL Terminator */
> id_len++;
> /* Identifier Length */
>
Reviewed-by: Lee Duncan
Re: [PATCH v4 5/7] target: add device vendor_id configfs attribute
On 11/28/18 5:01 PM, David Disseldorp wrote:
> The vendor_id attribute will allow for the modification of the T10
> Vendor Identification string returned in inquiry responses. Its value
> can be viewed and modified via the ConfigFS path at:
> target/core/$backstore/$name/wwn/vendor_id
>
> "LIO-ORG" remains the default value, which is set when the backstore
> device is enabled.
>
> Signed-off-by: David Disseldorp
> ---
> drivers/target/target_core_configfs.c | 48
> +++
> 1 file changed, 48 insertions(+)
>
> diff --git a/drivers/target/target_core_configfs.c
> b/drivers/target/target_core_configfs.c
> index 9f49b1afd685..fc60c4db718d 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -1213,6 +1213,52 @@ static struct t10_wwn *to_t10_wwn(struct config_item
> *item)
> }
>
> /*
> + * STANDARD and VPD page 0x80 T10 Vendor Identification
> + */
> +static ssize_t target_wwn_vendor_id_show(struct config_item *item,
> + char *page)
> +{
> + return sprintf(page, "%s\n", _t10_wwn(item)->vendor[0]);
> +}
> +
> +static ssize_t target_wwn_vendor_id_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct t10_wwn *t10_wwn = to_t10_wwn(item);
> + struct se_device *dev = t10_wwn->t10_dev;
> + unsigned char buf[INQUIRY_VENDOR_LEN + 1];
> +
> + if (strlen(page) > INQUIRY_VENDOR_LEN) {
> + pr_err("Emulated T10 Vendor Identification exceeds"
> + " INQUIRY_VENDOR_LEN: " __stringify(INQUIRY_VENDOR_LEN)
> + "\n");
> + return -EOVERFLOW;
> + }
> + strlcpy(buf, page, sizeof(buf));
> + /*
> + * Check to see if any active exports exist. If they do exist, fail
> + * here as changing this information on the fly (underneath the
> + * initiator side OS dependent multipath code) could cause negative
> + * effects.
> + */
> + if (dev->export_count) {
> + pr_err("Unable to set T10 Vendor Identification while"
> + " active %d exports exist\n", dev->export_count);
> + return -EINVAL;
> + }
> +
> + /* Assume ASCII encoding. Strip any newline added from userspace. */
> + BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
> + strlcpy(dev->t10_wwn.vendor, strstrip(buf),
> + sizeof(dev->t10_wwn.vendor));
> +
> + pr_debug("Target_Core_ConfigFS: Set emulated T10 Vendor Identification:"
> + " %s\n", dev->t10_wwn.vendor);
> +
> + return count;
> +}
> +
> +/*
> * VPD page 0x80 Unit serial
> */
> static ssize_t target_wwn_vpd_unit_serial_show(struct config_item *item,
> @@ -1358,6 +1404,7 @@ DEF_DEV_WWN_ASSOC_SHOW(vpd_assoc_target_port, 0x10);
> /* VPD page 0x83 Association: SCSI Target Device */
> DEF_DEV_WWN_ASSOC_SHOW(vpd_assoc_scsi_target_device, 0x20);
>
> +CONFIGFS_ATTR(target_wwn_, vendor_id);
> CONFIGFS_ATTR(target_wwn_, vpd_unit_serial);
> CONFIGFS_ATTR_RO(target_wwn_, vpd_protocol_identifier);
> CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_logical_unit);
> @@ -1365,6 +1412,7 @@ CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_target_port);
> CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_scsi_target_device);
>
> static struct configfs_attribute *target_core_dev_wwn_attrs[] = {
> + _wwn_attr_vendor_id,
> _wwn_attr_vpd_unit_serial,
> _wwn_attr_vpd_protocol_identifier,
> _wwn_attr_vpd_assoc_logical_unit,
>
Reviewed-by: Lee Duncan
Re: [PATCH v4 4/7] target: consistently null-terminate t10_wwn.revision
On 11/28/18 5:01 PM, David Disseldorp wrote:
> The pscsi_set_inquiry_info() codepath doesn't currently explicitly
> null-terminate t10_wwn.revision.
> Add an extra byte to the t10_wwn.model buffer and perform null string
> termination in all cases.
>
> Signed-off-by: David Disseldorp
> ---
> drivers/target/target_core_device.c | 6 --
> drivers/target/target_core_pscsi.c | 4 +++-
> drivers/target/target_core_spc.c| 5 +++--
> drivers/target/target_core_stat.c | 4 ++--
> include/target/target_core_base.h | 3 ++-
> 5 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/target/target_core_device.c
> b/drivers/target/target_core_device.c
> index 0d7382efb2d4..b3d0bd1ab09f 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -741,7 +741,7 @@ static void scsi_dump_inquiry(struct se_device *dev)
> buf[i] = '\0';
> pr_debug(" Model: %s\n", buf);
>
> - for (i = 0; i < 4; i++)
> + for (i = 0; i < INQUIRY_REVISION_LEN; i++)
> if (wwn->revision[i] >= 0x20)
> buf[i] = wwn->revision[i];
> else
> @@ -1010,6 +1010,7 @@ int target_configure_device(struct se_device *dev)
>*/
> 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)) {
> strncpy(>t10_wwn.vendor[0], "LIO-ORG", INQUIRY_VENDOR_LEN);
> dev->t10_wwn.vendor[INQUIRY_VENDOR_LEN] = '\0';
> @@ -1017,7 +1018,8 @@ int target_configure_device(struct se_device *dev)
> dev->transport->inquiry_prod, INQUIRY_MODEL_LEN);
> dev->t10_wwn.model[INQUIRY_MODEL_LEN] = '\0';
> strncpy(>t10_wwn.revision[0],
> - dev->transport->inquiry_rev, 4);
> + dev->transport->inquiry_rev, INQUIRY_REVISION_LEN);
> + dev->t10_wwn.revision[INQUIRY_REVISION_LEN] = '\0';
> }
>
> scsi_dump_inquiry(dev);
> diff --git a/drivers/target/target_core_pscsi.c
> b/drivers/target/target_core_pscsi.c
> index 1633babc2d4e..5493f620b7f4 100644
> --- a/drivers/target/target_core_pscsi.c
> +++ b/drivers/target/target_core_pscsi.c
> @@ -196,7 +196,9 @@ pscsi_set_inquiry_info(struct scsi_device *sdev, struct
> t10_wwn *wwn)
> BUILD_BUG_ON(sizeof(wwn->model) != INQUIRY_MODEL_LEN + 1);
> memcpy(>model[0], [16], INQUIRY_MODEL_LEN);
> wwn->model[INQUIRY_MODEL_LEN] = '\0';
> - memcpy(>revision[0], [32], sizeof(wwn->revision));
> + BUILD_BUG_ON(sizeof(wwn->revision) != INQUIRY_REVISION_LEN + 1);
> + memcpy(>revision[0], [32], INQUIRY_REVISION_LEN);
> + wwn->revision[INQUIRY_REVISION_LEN] = '\0';
> }
>
> static int
> diff --git a/drivers/target/target_core_spc.c
> b/drivers/target/target_core_spc.c
> index 78eddee4b6e6..8ffe712cb44d 100644
> --- a/drivers/target/target_core_spc.c
> +++ b/drivers/target/target_core_spc.c
> @@ -113,12 +113,13 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned
> char *buf)
>* unused bytes at the end of the field (i.e., highest offset) and the
>* unused bytes shall be filled with ASCII space characters (20h).
>*/
> - memset([8], 0x20, 8 + 16 + 4);
> + memset([8], 0x20,
> +INQUIRY_VENDOR_LEN + INQUIRY_MODEL_LEN + INQUIRY_REVISION_LEN);
> memcpy([8], "LIO-ORG", sizeof("LIO-ORG") - 1);
> memcpy([16], dev->t10_wwn.model,
> strnlen(dev->t10_wwn.model, INQUIRY_MODEL_LEN));
> memcpy([32], dev->t10_wwn.revision,
> -strnlen(dev->t10_wwn.revision, 4));
> +strnlen(dev->t10_wwn.revision, INQUIRY_REVISION_LEN));
> buf[4] = 31; /* Set additional length to 31 */
>
> return 0;
> diff --git a/drivers/target/target_core_stat.c
> b/drivers/target/target_core_stat.c
> index 9123c5137da5..e437ba494865 100644
> --- a/drivers/target/target_core_stat.c
> +++ b/drivers/target/target_core_stat.c
> @@ -275,10 +275,10 @@ static ssize_t target_stat_lu_rev_show(struct
> config_item *item, char *page)
> {
> struct se_device *dev = to_stat_lu_dev(item);
> int i;
> - char str[sizeof(dev->t10_wwn.revision)+1];
> + char str[INQUIRY_REVISION_LEN+1];
>
> /* scsiLuRevisionId */
> - for (i = 0; i < sizeof(dev->t10_wwn.revision); i++)
> + for (i = 0; i < INQUIRY_REVISION_LEN; i++)
> str[i] = ISPRINT(dev->t10_wwn.revision[i]) ?
> dev->t10_wwn.revision[i] : ' ';
> str[i] = '\0';
> diff --git a/include/target/target_core_base.h
> b/include/target/target_core_base.h
> index cfc279686cf4..497853a09fee 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -48,6 +48,7 @@
>
> #define INQUIRY_VENDOR_LEN
Re: [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model
On 11/28/18 5:01 PM, David Disseldorp wrote:
> The pscsi_set_inquiry_info() and emulate_model_alias_store() codepaths
> don't currently explicitly null-terminate t10_wwn.model.
> Add an extra byte to the t10_wwn.model buffer and perform null string
> termination in all cases.
>
> dev_set_t10_wwn_model_alias() continues to truncate at the same length
> to avoid changing the model string for existing deployments.
>
> Signed-off-by: David Disseldorp
> ---
> drivers/target/target_core_configfs.c | 8 +---
> drivers/target/target_core_device.c | 8 +---
> drivers/target/target_core_pscsi.c| 6 --
> drivers/target/target_core_spc.c | 2 +-
> drivers/target/target_core_stat.c | 4 ++--
> include/target/target_core_base.h | 3 ++-
> 6 files changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/target/target_core_configfs.c
> b/drivers/target/target_core_configfs.c
> index f6b1549f4142..9f49b1afd685 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -613,12 +613,12 @@ static void dev_set_t10_wwn_model_alias(struct
> se_device *dev)
> const char *configname;
>
> configname = config_item_name(>dev_group.cg_item);
> - if (strlen(configname) >= 16) {
> + if (strlen(configname) >= INQUIRY_MODEL_LEN) {
> pr_warn("dev[%p]: Backstore name '%s' is too long for "
> "INQUIRY_MODEL, truncating to 16 bytes\n", dev,
> configname);
> }
> - snprintf(>t10_wwn.model[0], 16, "%s", configname);
> + snprintf(>t10_wwn.model[0], INQUIRY_MODEL_LEN, "%s", configname);
> }
>
> static ssize_t emulate_model_alias_store(struct config_item *item,
> @@ -640,11 +640,13 @@ static ssize_t emulate_model_alias_store(struct
> config_item *item,
> if (ret < 0)
> return ret;
>
> + BUILD_BUG_ON(sizeof(dev->t10_wwn.model) != INQUIRY_MODEL_LEN + 1);
> if (flag) {
> dev_set_t10_wwn_model_alias(dev);
> } else {
> strncpy(>t10_wwn.model[0],
> - dev->transport->inquiry_prod, 16);
> + dev->transport->inquiry_prod, INQUIRY_MODEL_LEN);
> + dev->t10_wwn.model[INQUIRY_MODEL_LEN] = '\0';
> }
> da->emulate_model_alias = flag;
> return count;
> diff --git a/drivers/target/target_core_device.c
> b/drivers/target/target_core_device.c
> index fe4c4db51137..0d7382efb2d4 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -720,7 +720,7 @@ void core_dev_free_initiator_node_lun_acl(
> static void scsi_dump_inquiry(struct se_device *dev)
> {
> struct t10_wwn *wwn = >t10_wwn;
> - char buf[17];
> + char buf[INQUIRY_MODEL_LEN + 1];
> int i, device_type;
> /*
>* Print Linux/SCSI style INQUIRY formatting to the kernel ring buffer
> @@ -733,7 +733,7 @@ static void scsi_dump_inquiry(struct se_device *dev)
> buf[i] = '\0';
> pr_debug(" Vendor: %s\n", buf);
>
> - for (i = 0; i < 16; i++)
> + for (i = 0; i < INQUIRY_MODEL_LEN; i++)
> if (wwn->model[i] >= 0x20)
> buf[i] = wwn->model[i];
> else
> @@ -1009,11 +1009,13 @@ int target_configure_device(struct se_device *dev)
>* 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);
> if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
> strncpy(>t10_wwn.vendor[0], "LIO-ORG", INQUIRY_VENDOR_LEN);
> dev->t10_wwn.vendor[INQUIRY_VENDOR_LEN] = '\0';
> strncpy(>t10_wwn.model[0],
> - dev->transport->inquiry_prod, 16);
> + dev->transport->inquiry_prod, INQUIRY_MODEL_LEN);
> + dev->t10_wwn.model[INQUIRY_MODEL_LEN] = '\0';
> strncpy(>t10_wwn.revision[0],
> dev->transport->inquiry_rev, 4);
> }
> diff --git a/drivers/target/target_core_pscsi.c
> b/drivers/target/target_core_pscsi.c
> index ee65b5bb674c..1633babc2d4e 100644
> --- a/drivers/target/target_core_pscsi.c
> +++ b/drivers/target/target_core_pscsi.c
> @@ -193,7 +193,9 @@ pscsi_set_inquiry_info(struct scsi_device *sdev, struct
> t10_wwn *wwn)
> BUILD_BUG_ON(sizeof(wwn->vendor) != INQUIRY_VENDOR_LEN + 1);
> memcpy(>vendor[0], [8], INQUIRY_VENDOR_LEN);
> wwn->vendor[INQUIRY_VENDOR_LEN] = '\0';
> - memcpy(>model[0], [16], sizeof(wwn->model));
> + BUILD_BUG_ON(sizeof(wwn->model) != INQUIRY_MODEL_LEN + 1);
> + memcpy(>model[0], [16], INQUIRY_MODEL_LEN);
> + wwn->model[INQUIRY_MODEL_LEN] = '\0';
> memcpy(>revision[0], [32], sizeof(wwn->revision));
> }
>
> @@ -835,7 +837,7 @@ static ssize_t pscsi_show_configfs_dev_params(struct
>
Re: [PATCH v4 2/7] target: consistently null-terminate t10_wwn.vendor
On 11/28/18 5:01 PM, David Disseldorp wrote:
> In preparation for supporting user provided vendor strings, add an extra
> byte to t10_wwn.vendor which will ensure null string termination when an
> eight byte vendor string is provided by the user.
>
> Signed-off-by: David Disseldorp
> ---
> drivers/target/target_core_device.c | 6 --
> drivers/target/target_core_pscsi.c | 6 --
> drivers/target/target_core_stat.c | 4 ++--
> include/target/target_core_base.h | 8 +++-
> 4 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/target/target_core_device.c
> b/drivers/target/target_core_device.c
> index 47b5ef153135..fe4c4db51137 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -725,7 +725,7 @@ static void scsi_dump_inquiry(struct se_device *dev)
> /*
>* Print Linux/SCSI style INQUIRY formatting to the kernel ring buffer
>*/
> - for (i = 0; i < 8; i++)
> + for (i = 0; i < INQUIRY_VENDOR_LEN; i++)
> if (wwn->vendor[i] >= 0x20)
> buf[i] = wwn->vendor[i];
> else
> @@ -1008,8 +1008,10 @@ int target_configure_device(struct se_device *dev)
>* 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);
> if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
> - strncpy(>t10_wwn.vendor[0], "LIO-ORG", 8);
> + strncpy(>t10_wwn.vendor[0], "LIO-ORG", INQUIRY_VENDOR_LEN);
> + dev->t10_wwn.vendor[INQUIRY_VENDOR_LEN] = '\0';
> strncpy(>t10_wwn.model[0],
> dev->transport->inquiry_prod, 16);
> strncpy(>t10_wwn.revision[0],
> diff --git a/drivers/target/target_core_pscsi.c
> b/drivers/target/target_core_pscsi.c
> index 47d76c862014..ee65b5bb674c 100644
> --- a/drivers/target/target_core_pscsi.c
> +++ b/drivers/target/target_core_pscsi.c
> @@ -190,7 +190,9 @@ pscsi_set_inquiry_info(struct scsi_device *sdev, struct
> t10_wwn *wwn)
> /*
>* Use sdev->inquiry from drivers/scsi/scsi_scan.c:scsi_alloc_sdev()
>*/
> - memcpy(>vendor[0], [8], sizeof(wwn->vendor));
> + BUILD_BUG_ON(sizeof(wwn->vendor) != INQUIRY_VENDOR_LEN + 1);
> + memcpy(>vendor[0], [8], INQUIRY_VENDOR_LEN);
> + wwn->vendor[INQUIRY_VENDOR_LEN] = '\0';
> memcpy(>model[0], [16], sizeof(wwn->model));
> memcpy(>revision[0], [32], sizeof(wwn->revision));
> }
> @@ -826,7 +828,7 @@ static ssize_t pscsi_show_configfs_dev_params(struct
> se_device *dev, char *b)
> if (sd) {
> bl += sprintf(b + bl, "");
> bl += sprintf(b + bl, "Vendor: ");
> - for (i = 0; i < 8; i++) {
> + for (i = 0; i < INQUIRY_VENDOR_LEN; i++) {
> if (ISPRINT(sd->vendor[i])) /* printable character? */
> bl += sprintf(b + bl, "%c", sd->vendor[i]);
> else
> diff --git a/drivers/target/target_core_stat.c
> b/drivers/target/target_core_stat.c
> index f0db91ebd735..4210cf625d84 100644
> --- a/drivers/target/target_core_stat.c
> +++ b/drivers/target/target_core_stat.c
> @@ -247,10 +247,10 @@ static ssize_t target_stat_lu_vend_show(struct
> config_item *item, char *page)
> {
> struct se_device *dev = to_stat_lu_dev(item);
> int i;
> - char str[sizeof(dev->t10_wwn.vendor)+1];
> + char str[INQUIRY_VENDOR_LEN+1];
>
> /* scsiLuVendorId */
> - for (i = 0; i < sizeof(dev->t10_wwn.vendor); i++)
> + for (i = 0; i < INQUIRY_VENDOR_LEN; i++)
> str[i] = ISPRINT(dev->t10_wwn.vendor[i]) ?
> dev->t10_wwn.vendor[i] : ' ';
> str[i] = '\0';
> diff --git a/include/target/target_core_base.h
> b/include/target/target_core_base.h
> index e3bdb0550a59..cb1f3f574e2a 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -46,6 +46,8 @@
> /* Used by transport_get_inquiry_vpd_device_ident() */
> #define INQUIRY_VPD_DEVICE_IDENTIFIER_LEN254
>
> +#define INQUIRY_VENDOR_LEN 8
> +
> /* Attempts before moving from SHORT to LONG */
> #define PYX_TRANSPORT_WINDOW_CLOSED_THRESHOLD3
> #define PYX_TRANSPORT_WINDOW_CLOSED_WAIT_SHORT 3 /* In milliseconds */
> @@ -314,7 +316,11 @@ struct t10_vpd {
> };
>
> struct t10_wwn {
> - char vendor[8];
> + /*
> + * SCSI left aligned strings may not be null terminated. +1 to ensure a
> + * null terminator is always present.
> + */
> + char vendor[INQUIRY_VENDOR_LEN + 1];
> char model[16];
> char revision[4];
> char unit_serial[INQUIRY_VPD_SERIAL_LEN];
>
Reviewed-by: Lee Duncan
Re: [PATCH v4 2/7] target: consistently null-terminate t10_wwn.vendor
> On Nov 28, 2018, at 7:01 PM, David Disseldorp wrote: > > In preparation for supporting user provided vendor strings, add an extra > byte to t10_wwn.vendor which will ensure null string termination when an > eight byte vendor string is provided by the user. > > Signed-off-by: David Disseldorp > --- > drivers/target/target_core_device.c | 6 -- > drivers/target/target_core_pscsi.c | 6 -- > drivers/target/target_core_stat.c | 4 ++-- > include/target/target_core_base.h | 8 +++- > 4 files changed, 17 insertions(+), 7 deletions(-) > > Reviewed-by: Bryant G. Ly b...@catalogicsoftware.com
Re: [PATCH v4 1/7] target: use consistent left-aligned ASCII INQUIRY data
> On Nov 28, 2018, at 7:01 PM, David Disseldorp wrote: > > spc5r17.pdf specifies: > 4.3.1 ASCII data field requirements > ASCII data fields shall contain only ASCII printable characters (i.e., > code values 20h to 7Eh) and may be terminated with one or more ASCII > null (00h) characters. > ASCII data fields described as being left-aligned shall have any > unused bytes at the end of the field (i.e., highest offset) and the > unused bytes shall be filled with ASCII space characters (20h). > > LIO currently space-pads the T10 VENDOR IDENTIFICATION and PRODUCT > IDENTIFICATION fields in the standard INQUIRY data. However, the > PRODUCT REVISION LEVEL field in the standard INQUIRY data as well as the > T10 VENDOR IDENTIFICATION field in the INQUIRY Device Identification VPD > Page are zero-terminated/zero-padded. > > Fix this inconsistency by using space-padding for all of the above > fields. > > Signed-off-by: David Disseldorp > Reviewed-by: Christoph Hellwig > --- > drivers/target/target_core_spc.c | 17 - > 1 file changed, 12 insertions(+), 5 deletions(-) > Reviewed-by: Bryant G. Ly b...@catalogicsoftware.com
Re: [PATCH v4 1/7] target: use consistent left-aligned ASCII INQUIRY data
On 11/28/18 5:01 PM, David Disseldorp wrote:
> spc5r17.pdf specifies:
> 4.3.1 ASCII data field requirements
> ASCII data fields shall contain only ASCII printable characters (i.e.,
> code values 20h to 7Eh) and may be terminated with one or more ASCII
> null (00h) characters.
> ASCII data fields described as being left-aligned shall have any
> unused bytes at the end of the field (i.e., highest offset) and the
> unused bytes shall be filled with ASCII space characters (20h).
>
> LIO currently space-pads the T10 VENDOR IDENTIFICATION and PRODUCT
> IDENTIFICATION fields in the standard INQUIRY data. However, the
> PRODUCT REVISION LEVEL field in the standard INQUIRY data as well as the
> T10 VENDOR IDENTIFICATION field in the INQUIRY Device Identification VPD
> Page are zero-terminated/zero-padded.
>
> Fix this inconsistency by using space-padding for all of the above
> fields.
>
> Signed-off-by: David Disseldorp
> Reviewed-by: Christoph Hellwig
> ---
> drivers/target/target_core_spc.c | 17 -
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/target/target_core_spc.c
> b/drivers/target/target_core_spc.c
> index f459118bc11b..c37dd36ec77d 100644
> --- a/drivers/target/target_core_spc.c
> +++ b/drivers/target/target_core_spc.c
> @@ -108,12 +108,17 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned
> char *buf)
>
> buf[7] = 0x2; /* CmdQue=1 */
>
> - memcpy([8], "LIO-ORG ", 8);
> - memset([16], 0x20, 16);
> + /*
> + * ASCII data fields described as being left-aligned shall have any
> + * unused bytes at the end of the field (i.e., highest offset) and the
> + * unused bytes shall be filled with ASCII space characters (20h).
> + */
> + memset([8], 0x20, 8 + 16 + 4);
I dislike that you are using 0x20 here (and below) instead of ' '.
> + memcpy([8], "LIO-ORG", sizeof("LIO-ORG") - 1);
> memcpy([16], dev->t10_wwn.model,
> -min_t(size_t, strlen(dev->t10_wwn.model), 16));
> +strnlen(dev->t10_wwn.model, 16));
> memcpy([32], dev->t10_wwn.revision,
> -min_t(size_t, strlen(dev->t10_wwn.revision), 4));
> +strnlen(dev->t10_wwn.revision, 4));
> buf[4] = 31; /* Set additional length to 31 */
>
> return 0;
> @@ -251,7 +256,9 @@ spc_emulate_evpd_83(struct se_cmd *cmd, unsigned char
> *buf)
> buf[off] = 0x2; /* ASCII */
> buf[off+1] = 0x1; /* T10 Vendor ID */
> buf[off+2] = 0x0;
> - memcpy([off+4], "LIO-ORG", 8);
> + /* left align Vendor ID and pad with spaces */
> + memset([off+4], 0x20, 8);
> + memcpy([off+4], "LIO-ORG", sizeof("LIO-ORG") - 1);
> /* Extra Byte for NULL Terminator */
> id_len++;
> /* Identifier Length */
>
Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response
> On Nov 28, 2018, at 7:01 PM, David Disseldorp wrote: > > Use the value stored in t10_wwn.vendor, which defaults to "LIO-ORG", but > can be reconfigured via the vendor_id ConfigFS attribute. > > Signed-off-by: David Disseldorp > --- > drivers/target/target_core_spc.c | 8 +--- > 1 file changed, 5 insertions(+), 3 deletions(-) > Reviewed-by: Bryant G. Ly b...@catalogicsoftware.com
Re: [PATCH v4 5/7] target: add device vendor_id configfs attribute
> On Nov 28, 2018, at 7:01 PM, David Disseldorp wrote: > > The vendor_id attribute will allow for the modification of the T10 > Vendor Identification string returned in inquiry responses. Its value > can be viewed and modified via the ConfigFS path at: > target/core/$backstore/$name/wwn/vendor_id > > "LIO-ORG" remains the default value, which is set when the backstore > device is enabled. > > Signed-off-by: David Disseldorp > --- > drivers/target/target_core_configfs.c | 48 +++ > 1 file changed, 48 insertions(+) > > diff --git a/drivers/target/target_core_configfs.c > b/drivers/target/target_core_configfs.c > index 9f49b1afd685..fc60c4db718d 100644 > --- a/drivers/target/target_core_configfs.c > +++ b/drivers/target/target_core_configfs.c > @@ -1213,6 +1213,52 @@ static struct t10_wwn *to_t10_wwn(struct config_item > *item) > } > Thanks for making it configurable. I know back when I was at IBM we made changes internally to support LIO-ORG. Reviewed-by: Bryant G. Ly b...@catalogicsoftware.com
Re: [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model
> On Nov 28, 2018, at 7:01 PM, David Disseldorp wrote: > > The pscsi_set_inquiry_info() and emulate_model_alias_store() codepaths > don't currently explicitly null-terminate t10_wwn.model. > Add an extra byte to the t10_wwn.model buffer and perform null string > termination in all cases. > > dev_set_t10_wwn_model_alias() continues to truncate at the same length > to avoid changing the model string for existing deployments. > > Signed-off-by: David Disseldorp > --- > drivers/target/target_core_configfs.c | 8 +--- > drivers/target/target_core_device.c | 8 +--- > drivers/target/target_core_pscsi.c| 6 -- > drivers/target/target_core_spc.c | 2 +- > drivers/target/target_core_stat.c | 4 ++-- > include/target/target_core_base.h | 3 ++- > 6 files changed, 19 insertions(+), 12 deletions(-) > > Reviewed-by: Bryant G. Ly b...@catalogicsoftware.com
Re: [PATCH v4 4/7] target: consistently null-terminate t10_wwn.revision
> On Nov 28, 2018, at 7:01 PM, David Disseldorp wrote: > > The pscsi_set_inquiry_info() codepath doesn't currently explicitly > null-terminate t10_wwn.revision. > Add an extra byte to the t10_wwn.model buffer and perform null string > termination in all cases. > > Signed-off-by: David Disseldorp > --- > drivers/target/target_core_device.c | 6 -- > drivers/target/target_core_pscsi.c | 4 +++- > drivers/target/target_core_spc.c| 5 +++-- > drivers/target/target_core_stat.c | 4 ++-- > include/target/target_core_base.h | 3 ++- > 5 files changed, 14 insertions(+), 8 deletions(-) > Reviewed-by: Bryant G. Ly b...@catalogicsoftware.com
Re: [PATCH v2 8/9] qedi: Move LL2 producer index processing in BH.
On 11/21/18 1:25 AM, Nilesh Javali wrote:
> From: Manish Rangankar
>
> 1. Removed logic to update HW producer index in interrupt context.
> 2. Update HW producer index after UIO ring and buffer gets initialized.
>
> Signed-off-by: Manish Rangankar
> ---
> drivers/scsi/qedi/qedi_main.c | 31 +++
> 1 file changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 8942f62..053a947 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -665,7 +665,6 @@ static int qedi_ll2_rx(void *cookie, struct sk_buff *skb,
> u32 arg1, u32 arg2)
> struct qedi_uio_ctrl *uctrl;
> struct skb_work_list *work;
> struct ethhdr *eh;
> - u32 prod;
>
> if (!qedi) {
> QEDI_ERR(NULL, "qedi is NULL\n");
> @@ -724,17 +723,10 @@ static int qedi_ll2_rx(void *cookie, struct sk_buff
> *skb, u32 arg1, u32 arg2)
>
> spin_lock_bh(>ll2_lock);
> list_add_tail(>list, >ll2_skb_list);
> + spin_unlock_bh(>ll2_lock);
>
> - ++uctrl->hw_rx_prod_cnt;
> - prod = (uctrl->hw_rx_prod + 1) % RX_RING;
> - if (prod != uctrl->host_rx_cons) {
> - uctrl->hw_rx_prod = prod;
> - spin_unlock_bh(>ll2_lock);
> - wake_up_process(qedi->ll2_recv_thread);
> - return 0;
> - }
> + wake_up_process(qedi->ll2_recv_thread);
>
> - spin_unlock_bh(>ll2_lock);
> return 0;
> }
>
> @@ -749,6 +741,7 @@ static int qedi_ll2_process_skb(struct qedi_ctx *qedi,
> struct sk_buff *skb,
> u32 rx_bd_prod;
> void *pkt;
> int len = 0;
> + u32 prod;
>
> if (!qedi) {
> QEDI_ERR(NULL, "qedi is NULL\n");
> @@ -757,12 +750,16 @@ static int qedi_ll2_process_skb(struct qedi_ctx *qedi,
> struct sk_buff *skb,
>
> udev = qedi->udev;
> uctrl = udev->uctrl;
> - pkt = udev->rx_pkt + (uctrl->hw_rx_prod * qedi_ll2_buf_size);
> +
> + ++uctrl->hw_rx_prod_cnt;
> + prod = (uctrl->hw_rx_prod + 1) % RX_RING;
> +
> + pkt = udev->rx_pkt + (prod * qedi_ll2_buf_size);
> len = min_t(u32, skb->len, (u32)qedi_ll2_buf_size);
> memcpy(pkt, skb->data, len);
>
> memset(, 0, sizeof(rxbd));
> - rxbd.rx_pkt_index = uctrl->hw_rx_prod;
> + rxbd.rx_pkt_index = prod;
> rxbd.rx_pkt_len = len;
> rxbd.vlan_id = vlan_id;
>
> @@ -773,6 +770,16 @@ static int qedi_ll2_process_skb(struct qedi_ctx *qedi,
> struct sk_buff *skb,
>
> memcpy(p_rxbd, , sizeof(rxbd));
>
> + QEDI_INFO(>dbg_ctx, QEDI_LOG_LL2,
> + "hw_rx_prod [%d] prod [%d] hw_rx_bd_prod [%d] rx_pkt_idx [%d]
> rx_len [%d].\n",
> + uctrl->hw_rx_prod, prod, uctrl->hw_rx_bd_prod,
> + rxbd.rx_pkt_index, rxbd.rx_pkt_len);
> + QEDI_INFO(>dbg_ctx, QEDI_LOG_LL2,
> + "host_rx_cons [%d] hw_rx_bd_cons [%d].\n",
> + uctrl->host_rx_cons, uctrl->host_rx_bd_cons);
> +
> + uctrl->hw_rx_prod = prod;
> +
> /* notify the iscsiuio about new packet */
> uio_event_notify(>qedi_uinfo);
>
>
Reviewed-by: Lee Duncan
Re: [PATCH v2 7/9] qedi: add module param to set ping packet size
On 11/21/18 1:25 AM, Nilesh Javali wrote: > Default packet size is 0x400. > For jumbo packets set to 0x2400. > > Signed-off-by: Nilesh Javali > --- > drivers/scsi/qedi/qedi.h | 1 - > drivers/scsi/qedi/qedi_main.c | 13 + > 2 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/scsi/qedi/qedi.h b/drivers/scsi/qedi/qedi.h > index 6fa02c5..a26bb506 100644 > --- a/drivers/scsi/qedi/qedi.h > +++ b/drivers/scsi/qedi/qedi.h > @@ -63,7 +63,6 @@ > #define QEDI_LOCAL_PORT_INVALID 0x > #define TX_RX_RING 16 > #define RX_RING (TX_RX_RING - 1) > -#define LL2_SINGLE_BUF_SIZE 0x400 > #define QEDI_PAGE_ALIGN(addr)ALIGN(addr, QEDI_PAGE_SIZE) > #define QEDI_PAGE_MASK (~((QEDI_PAGE_SIZE) - 1)) > > diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c > index 2621dee..8942f62 100644 > --- a/drivers/scsi/qedi/qedi_main.c > +++ b/drivers/scsi/qedi/qedi_main.c > @@ -44,6 +44,11 @@ > MODULE_PARM_DESC(qedi_io_tracing, >" Enable logging of SCSI requests/completions into trace > buffer. (default off)."); > > +uint qedi_ll2_buf_size = 0x400; > +module_param(qedi_ll2_buf_size, uint, 0644); > +MODULE_PARM_DESC(qedi_ll2_buf_size, > + "parameter to set ping packet size, default - 0x400, Jumbo > packets - 0x2400."); > + > const struct qed_iscsi_ops *qedi_ops; > static struct scsi_transport_template *qedi_scsi_transport; > static struct pci_driver qedi_pci_driver; > @@ -228,7 +233,7 @@ static int __qedi_alloc_uio_rings(struct qedi_uio_dev > *udev) > } > > /* Allocating memory for Tx/Rx pkt buffer */ > - udev->ll2_buf_size = TX_RX_RING * LL2_SINGLE_BUF_SIZE; > + udev->ll2_buf_size = TX_RX_RING * qedi_ll2_buf_size; > udev->ll2_buf_size = QEDI_PAGE_ALIGN(udev->ll2_buf_size); > udev->ll2_buf = (void *)__get_free_pages(GFP_KERNEL | __GFP_COMP | >__GFP_ZERO, 2); > @@ -283,7 +288,7 @@ static int qedi_alloc_uio_rings(struct qedi_ctx *qedi) > qedi->udev = udev; > > udev->tx_pkt = udev->ll2_buf; > - udev->rx_pkt = udev->ll2_buf + LL2_SINGLE_BUF_SIZE; > + udev->rx_pkt = udev->ll2_buf + qedi_ll2_buf_size; > return 0; > > err_uctrl: > @@ -752,8 +757,8 @@ static int qedi_ll2_process_skb(struct qedi_ctx *qedi, > struct sk_buff *skb, > > udev = qedi->udev; > uctrl = udev->uctrl; > - pkt = udev->rx_pkt + (uctrl->hw_rx_prod * LL2_SINGLE_BUF_SIZE); > - len = min_t(u32, skb->len, (u32)LL2_SINGLE_BUF_SIZE); > + pkt = udev->rx_pkt + (uctrl->hw_rx_prod * qedi_ll2_buf_size); > + len = min_t(u32, skb->len, (u32)qedi_ll2_buf_size); > memcpy(pkt, skb->data, len); > > memset(, 0, sizeof(rxbd)); > Reviewed-by: Lee Duncan
Re: [PATCH v2 6/9] qedi: Add packet filter in light L2 Rx path.
On 11/21/18 1:25 AM, Nilesh Javali wrote:
> From: Manish Rangankar
>
> Add packet filter to avoid unnecessary packet processing in iscsiuio.
>
> Signed-off-by: Manish Rangankar
> ---
> drivers/scsi/qedi/qedi_main.c | 24
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 713db9c..2621dee 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -659,6 +659,7 @@ static int qedi_ll2_rx(void *cookie, struct sk_buff *skb,
> u32 arg1, u32 arg2)
> struct qedi_uio_dev *udev;
> struct qedi_uio_ctrl *uctrl;
> struct skb_work_list *work;
> + struct ethhdr *eh;
> u32 prod;
>
> if (!qedi) {
> @@ -673,6 +674,29 @@ static int qedi_ll2_rx(void *cookie, struct sk_buff
> *skb, u32 arg1, u32 arg2)
> return 0;
> }
>
> + eh = (struct ethhdr *)skb->data;
> + /* Undo VLAN encapsulation */
> + if (eh->h_proto == htons(ETH_P_8021Q)) {
> + memmove((u8 *)eh + VLAN_HLEN, eh, ETH_ALEN * 2);
> + eh = (struct ethhdr *)skb_pull(skb, VLAN_HLEN);
> + skb_reset_mac_header(skb);
> + }
> +
> + /* Filter out non FIP/FCoE frames here to free them faster */
> + if (eh->h_proto != htons(ETH_P_ARP) &&
> + eh->h_proto != htons(ETH_P_IP) &&
> + eh->h_proto != htons(ETH_P_IPV6)) {
> + QEDI_INFO(>dbg_ctx, QEDI_LOG_LL2,
> + "Dropping frame ethertype [0x%x] len [0x%x].\n",
> + eh->h_proto, skb->len);
> + kfree_skb(skb);
> + return 0;
> + }
> +
> + QEDI_INFO(>dbg_ctx, QEDI_LOG_LL2,
> + "Allowed frame ethertype [0x%x] len [0x%x].\n",
> + eh->h_proto, skb->len);
> +
> udev = qedi->udev;
> uctrl = udev->uctrl;
>
>
Reviewed-by: Lee Duncan
Re: [PATCH v2 5/9] qedi: Check for session online before getting iSCSI TLV data.
On 11/21/18 1:25 AM, Nilesh Javali wrote:
> From: Manish Rangankar
>
> The kernel panic was observed after switch side perturbation,
>
> BUG: unable to handle kernel NULL pointer dereference at (null)
> IP: [] strcmp+0x20/0x40
> PGD 0 Oops: [#1] SMP
> CPU: 8 PID: 647 Comm: kworker/8:1 Tainted: GW OE
> 3.10.0-693.el7.x86_64 #1
> Hardware name: HPE ProLiant DL380 Gen10/ProLiant DL380 Gen10, BIOS U30
> 06/20/2018
> Workqueue: slowpath-13:00. qed_slowpath_task [qed]
> task: 880429eb8fd0 ti: 88042919 task.ti: 88042919
> RIP: 0010:[] [] strcmp+0x20/0x40
> RSP: 0018:880429193c68 EFLAGS: 00010202
> RAX: 000a RBX: 0002 RCX:
> RDX: 0001 RSI: 0001 RDI: 88042bda7a41
> RBP: 880429193c68 R08: R09:
> R10: 0007 R11: 88042b3af338 R12: 880420b007a0
> R13: 88081aa56af8 R14: 0001 R15: 88081aa50410
> FS: () GS:88042fe0() knlGS:
> CS: 0010 DS: ES: CR0: 80050033
> CR2: CR3: 019f2000 CR4: 003407e0
> DR0: DR1: DR2:
> DR3: DR6: fffe0ff0 DR7: 0400
> Stack:
> 880429193d20 c02a0c90 c90004b32000 8803fd3ec600
> 88042bda7800 88042bda7a00 88042bda7840 88042bda7a40
> 000129193d10 2e3836312e323931 ff000a342e363232 c01ad99d
> Call Trace:
> [] qedi_get_protocol_tlv_data+0x270/0x470 [qedi]
> [] ? qed_mfw_process_tlv_req+0x24d/0xbf0 [qed]
> [] qed_mfw_fill_tlv_data+0x5e/0xd0 [qed]
> [] qed_mfw_process_tlv_req+0x269/0xbf0 [qed]
>
> Fix kernel NULL pointer deref by checking for session is online
> before getting iSCSI TLV data.
>
> Signed-off-by: Manish Rangankar
> ---
> drivers/scsi/qedi/qedi_main.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 5308e6b..713db9c 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -952,6 +952,9 @@ static int qedi_find_boot_info(struct qedi_ctx *qedi,
> cls_sess = iscsi_conn_to_session(cls_conn);
> sess = cls_sess->dd_data;
>
> + if (!iscsi_is_session_online(cls_sess))
> + continue;
> +
> if (pri_ctrl_flags) {
> if (!strcmp(pri_tgt->iscsi_name, sess->targetname) &&
> !strcmp(pri_tgt->ip_addr, ep_ip_addr)) {
>
Reviewed-by: Lee Duncan
Re: [PATCH v2 3/9] qedi: Replace PAGE_SIZE with QEDI_PAGE_SIZE
On 11/21/18 1:25 AM, Nilesh Javali wrote:
> Use QEDI_PAGE_SIZE for enablement of module on systems with 64K page size.
>
> Signed-off-by: Nilesh Javali
> ---
> drivers/scsi/qedi/qedi_main.c | 16 +---
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 0f8eb5f..a1225ae 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -796,7 +796,7 @@ static int qedi_set_iscsi_pf_param(struct qedi_ctx *qedi)
> int rval = 0;
>
>
> - num_sq_pages = (MAX_OUTSTANDING_TASKS_PER_CON * 8) / PAGE_SIZE;
> + num_sq_pages = (MAX_OUTSTANDING_TASKS_PER_CON * 8) / QEDI_PAGE_SIZE;
>
> qedi->num_queues = MIN_NUM_CPUS_MSIX(qedi);
>
> @@ -834,7 +834,7 @@ static int qedi_set_iscsi_pf_param(struct qedi_ctx *qedi)
> qedi->pf_params.iscsi_pf_params.max_fin_rt = 2;
>
> for (log_page_size = 0 ; log_page_size < 32 ; log_page_size++) {
> - if ((1 << log_page_size) == PAGE_SIZE)
> + if ((1 << log_page_size) == QEDI_PAGE_SIZE)
> break;
> }
> qedi->pf_params.iscsi_pf_params.log_page_size = log_page_size;
> @@ -1376,7 +1376,7 @@ static void qedi_free_bdq(struct qedi_ctx *qedi)
> int i;
>
> if (qedi->bdq_pbl_list)
> - dma_free_coherent(>pdev->dev, PAGE_SIZE,
> + dma_free_coherent(>pdev->dev, QEDI_PAGE_SIZE,
> qedi->bdq_pbl_list, qedi->bdq_pbl_list_dma);
>
> if (qedi->bdq_pbl)
> @@ -1437,7 +1437,7 @@ static int qedi_alloc_bdq(struct qedi_ctx *qedi)
>
> /* Alloc dma memory for BDQ page buffer list */
> qedi->bdq_pbl_mem_size = QEDI_BDQ_NUM * sizeof(struct scsi_bd);
> - qedi->bdq_pbl_mem_size = ALIGN(qedi->bdq_pbl_mem_size, PAGE_SIZE);
> + qedi->bdq_pbl_mem_size = ALIGN(qedi->bdq_pbl_mem_size, QEDI_PAGE_SIZE);
> qedi->rq_num_entries = qedi->bdq_pbl_mem_size / sizeof(struct scsi_bd);
>
> QEDI_INFO(>dbg_ctx, QEDI_LOG_CONN, "rq_num_entries = %d.\n",
> @@ -1472,7 +1472,8 @@ static int qedi_alloc_bdq(struct qedi_ctx *qedi)
> }
>
> /* Allocate list of PBL pages */
> - qedi->bdq_pbl_list = dma_zalloc_coherent(>pdev->dev, PAGE_SIZE,
> + qedi->bdq_pbl_list = dma_zalloc_coherent(>pdev->dev,
> + QEDI_PAGE_SIZE,
>>bdq_pbl_list_dma,
>GFP_KERNEL);
> if (!qedi->bdq_pbl_list) {
> @@ -1485,13 +1486,14 @@ static int qedi_alloc_bdq(struct qedi_ctx *qedi)
>* Now populate PBL list with pages that contain pointers to the
>* individual buffers.
>*/
> - qedi->bdq_pbl_list_num_entries = qedi->bdq_pbl_mem_size / PAGE_SIZE;
> + qedi->bdq_pbl_list_num_entries = qedi->bdq_pbl_mem_size /
> + QEDI_PAGE_SIZE;
> list = (u64 *)qedi->bdq_pbl_list;
> page = qedi->bdq_pbl_list_dma;
> for (i = 0; i < qedi->bdq_pbl_list_num_entries; i++) {
> *list = qedi->bdq_pbl_dma;
> list++;
> - page += PAGE_SIZE;
> + page += QEDI_PAGE_SIZE;
> }
>
> return 0;
>
Reviewed-by: Lee Duncan
Re: [PATCH v2 2/9] qedi: Fix spelling mistake "OUSTANDING" -> "OUTSTANDING"
On 11/21/18 1:25 AM, Nilesh Javali wrote:
> Fix trivial spelling mistake within macro definition.
>
> Signed-off-by: Nilesh Javali
> ---
> drivers/scsi/qedi/qedi.h | 4 ++--
> drivers/scsi/qedi/qedi_main.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/qedi/qedi.h b/drivers/scsi/qedi/qedi.h
> index e966855..6fa02c5 100644
> --- a/drivers/scsi/qedi/qedi.h
> +++ b/drivers/scsi/qedi/qedi.h
> @@ -45,7 +45,7 @@
> #define QEDI_MAX_TASK_NUM0x0FFF
> #define QEDI_MAX_ISCSI_CONNS_PER_HBA 1024
> #define QEDI_ISCSI_MAX_BDS_PER_CMD 255 /* Firmware max BDs is 255 */
> -#define MAX_OUSTANDING_TASKS_PER_CON 1024
> +#define MAX_OUTSTANDING_TASKS_PER_CON1024
>
> #define QEDI_MAX_BD_LEN 0x
> #define QEDI_BD_SPLIT_SZ 0x1000
> @@ -144,7 +144,7 @@ struct skb_work_list {
> };
>
> /* Queue sizes in number of elements */
> -#define QEDI_SQ_SIZE MAX_OUSTANDING_TASKS_PER_CON
> +#define QEDI_SQ_SIZE MAX_OUTSTANDING_TASKS_PER_CON
> #define QEDI_CQ_SIZE 2048
> #define QEDI_CMDQ_SIZE QEDI_MAX_ISCSI_TASK
> #define QEDI_PROTO_CQ_PROD_IDX 0
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 105b0e4..0f8eb5f 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -796,7 +796,7 @@ static int qedi_set_iscsi_pf_param(struct qedi_ctx *qedi)
> int rval = 0;
>
>
> - num_sq_pages = (MAX_OUSTANDING_TASKS_PER_CON * 8) / PAGE_SIZE;
> + num_sq_pages = (MAX_OUTSTANDING_TASKS_PER_CON * 8) / PAGE_SIZE;
>
> qedi->num_queues = MIN_NUM_CPUS_MSIX(qedi);
>
>
Reviewed-by: Lee Duncan
Re: [PATCH v2 1/9] qedi: Cleanup redundant QEDI_PAGE_SIZE macro definition
On 11/21/18 1:25 AM, Nilesh Javali wrote: > Remove redundant macro definition. > > Signed-off-by: Nilesh Javali > --- > drivers/scsi/qedi/qedi.h | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/scsi/qedi/qedi.h b/drivers/scsi/qedi/qedi.h > index a6f96b3..e966855 100644 > --- a/drivers/scsi/qedi/qedi.h > +++ b/drivers/scsi/qedi/qedi.h > @@ -64,11 +64,9 @@ > #define TX_RX_RING 16 > #define RX_RING (TX_RX_RING - 1) > #define LL2_SINGLE_BUF_SIZE 0x400 > -#define QEDI_PAGE_SIZE 4096 > #define QEDI_PAGE_ALIGN(addr)ALIGN(addr, QEDI_PAGE_SIZE) > #define QEDI_PAGE_MASK (~((QEDI_PAGE_SIZE) - 1)) > > -#define QEDI_PAGE_SIZE 4096 > #define QEDI_HW_DMA_BOUNDARY 0xfff > #define QEDI_PATH_HANDLE 0xFE000UL > > Reviewed-by: Lee Duncan
Re: [PATCH v2 4/9] qedi: Allocate IRQs based on msix_cnt
On 11/21/18 1:25 AM, Nilesh Javali wrote:
> The driver load on some systems failed with error,
> [0004:01:00.5]:[qedi_request_msix_irq:2524]:8: request_irq failed.
>
> Allocate the IRQs based on MSIX count obtained from qed module
> instead of number of queues.
>
> Signed-off-by: Nilesh Javali
> ---
> drivers/scsi/qedi/qedi_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index a1225ae..5308e6b 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -1298,7 +1298,7 @@ static int qedi_request_msix_irq(struct qedi_ctx *qedi)
> int i, rc, cpu;
>
> cpu = cpumask_first(cpu_online_mask);
> - for (i = 0; i < MIN_NUM_CPUS_MSIX(qedi); i++) {
> + for (i = 0; i < qedi->int_info.msix_cnt; i++) {
> rc = request_irq(qedi->int_info.msix[i].vector,
>qedi_msix_handler, 0, "qedi",
>>fp_array[i]);
>
Reviewed-by: Lee Duncan
Re: [PATCH 0/3] target: drop unneeded pi_prot_format and get_fabric_name()
David, > This patchset removes unneeded se_dev_attrib.pi_prot_format and > fabric_ops.get_fabric_name() struct members. Removal of the latter > allowed for further cleanup due to the fact that all fabric modules > except iscsi_target_mod provide matching strings for fabric_ops.name > and fabric_ops.get_fabric_name() - use a new fabric_ops.fabric_alias > member to handle the iscsi_target_mod special case. Applied to 4.21/scsi-queue. Thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 2/2] qla2xxx: Split the __qla2x00_abort_all_cmds() function
> On Nov 27, 2018, at 3:04 PM, Bart Van Assche wrote:
>
> External Email
>
> Nesting in __qla2x00_abort_all_cmds() is way too deep. Reduce the nesting
> level by introducing a helper function. This patch does not change any
> functionality.
>
> Cc: Himanshu Madhani
> Signed-off-by: Bart Van Assche
> ---
> drivers/scsi/qla2xxx/qla_os.c | 89 +--
> 1 file changed, 44 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index b658b9a5eb1e..247f16969f0f 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -1746,10 +1746,52 @@ qla2x00_loop_reset(scsi_qla_host_t *vha)
>return QLA_SUCCESS;
> }
>
> +static void qla2x00_abort_srb(struct qla_qpair *qp, srb_t *sp, const int res,
> + unsigned long *flags)
> + __releases(qp->qp_lock_ptr)
> + __acquires(qp->qp_lock_ptr)
> +{
> + scsi_qla_host_t *vha = qp->vha;
> + struct qla_hw_data *ha = vha->hw;
> + int status;
> +
> + if (sp->type == SRB_NVME_CMD || sp->type == SRB_NVME_LS) {
> + if (!sp_get(sp)) {
> + /* got sp */
> + spin_unlock_irqrestore(qp->qp_lock_ptr, *flags);
> + qla_nvme_abort(ha, sp, res);
> + spin_lock_irqsave(qp->qp_lock_ptr, *flags);
> + }
> + } else if (GET_CMD_SP(sp) && !ha->flags.eeh_busy &&
> + !test_bit(ABORT_ISP_ACTIVE, >dpc_flags) &&
> + !qla2x00_isp_reg_stat(ha) && sp->type == SRB_SCSI_CMD) {
> + /*
> +* Don't abort commands in adapter during EEH recovery as it's
> +* not accessible/responding.
> +*
> +* Get a reference to the sp and drop the lock. The reference
> +* ensures this sp->done() call and not the call in
> +* qla2xxx_eh_abort() ends the SCSI cmd (with result 'res').
> +*/
> + if (!sp_get(sp)) {
> + spin_unlock_irqrestore(qp->qp_lock_ptr, *flags);
> + status = qla2xxx_eh_abort(GET_CMD_SP(sp));
> + spin_lock_irqsave(qp->qp_lock_ptr, *flags);
> + /*
> +* Get rid of extra reference caused by early exit
> +* from qla2xxx_eh_abort().
> +*/
> + if (status == FAST_IO_FAIL)
> + atomic_dec(>ref_count);
> + }
> + }
> + sp->done(sp, res);
> +}
> +
> static void
> __qla2x00_abort_all_cmds(struct qla_qpair *qp, int res)
> {
> - int cnt, status;
> + int cnt;
>unsigned long flags;
>srb_t *sp;
>scsi_qla_host_t *vha = qp->vha;
> @@ -1768,50 +1810,7 @@ __qla2x00_abort_all_cmds(struct qla_qpair *qp, int res)
>req->outstanding_cmds[cnt] = NULL;
>switch (sp->cmd_type) {
>case TYPE_SRB:
> - if (sp->type == SRB_NVME_CMD ||
> - sp->type == SRB_NVME_LS) {
> - if (!sp_get(sp)) {
> - /* got sp */
> - spin_unlock_irqrestore
> - (qp->qp_lock_ptr,
> -flags);
> - qla_nvme_abort(ha, sp, res);
> - spin_lock_irqsave
> - (qp->qp_lock_ptr,
> flags);
> - }
> - } else if (GET_CMD_SP(sp) &&
> - !ha->flags.eeh_busy &&
> - (!test_bit(ABORT_ISP_ACTIVE,
> - >dpc_flags)) &&
> - !qla2x00_isp_reg_stat(ha) &&
> - (sp->type == SRB_SCSI_CMD)) {
> - /*
> -* Don't abort commands in adapter
> -* during EEH recovery as it's not
> -* accessible/responding.
> -*
> -* Get a reference to the sp and drop
> -* the lock. The reference ensures
> this
> -* sp->done() call and not the call in
> -* qla2xxx_eh_abort() ends the SCSI
> cmd
> -* (with result 'res').
> -
Re: [PATCH 1/2] qla2xxx: Introduce a switch/case statement in qlt_xmit_tm_rsp()
> On Nov 27, 2018, at 3:04 PM, Bart Van Assche wrote:
>
> External Email
>
> This patch improves code readability but does not change any
> functionality.
>
> Cc: Himanshu Madhani
> Signed-off-by: Bart Van Assche
> ---
> drivers/scsi/qla2xxx/qla_target.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/qla2xxx/qla_target.c
> b/drivers/scsi/qla2xxx/qla_target.c
> index c4504740f0e2..802007a7b21c 100644
> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c
> @@ -2379,20 +2379,20 @@ void qlt_xmit_tm_rsp(struct qla_tgt_mgmt_cmd *mcmd)
>}
>
>if (mcmd->flags == QLA24XX_MGMT_SEND_NACK) {
> - if (mcmd->orig_iocb.imm_ntfy.u.isp24.status_subcode ==
> - ELS_LOGO ||
> - mcmd->orig_iocb.imm_ntfy.u.isp24.status_subcode ==
> - ELS_PRLO ||
> - mcmd->orig_iocb.imm_ntfy.u.isp24.status_subcode ==
> - ELS_TPRLO) {
> + switch (mcmd->orig_iocb.imm_ntfy.u.isp24.status_subcode) {
> + case ELS_LOGO:
> + case ELS_PRLO:
> + case ELS_TPRLO:
>ql_dbg(ql_dbg_disc, vha, 0x2106,
>"TM response logo %phC status %#x state %#x",
>mcmd->sess->port_name, mcmd->fc_tm_rsp,
>mcmd->flags);
>qlt_schedule_sess_for_deletion(mcmd->sess);
> - } else {
> + break;
> + default:
>qlt_send_notify_ack(vha->hw->base_qpair,
>>orig_iocb.imm_ntfy, 0, 0, 0, 0, 0, 0);
> + break;
>}
>} else {
>if (mcmd->orig_iocb.atio.u.raw.entry_type == ABTS_RECV_24XX) {
> --
> 2.20.0.rc0.387.gc7a69e6b6c-goog
>
Looks Good.
Acked-by: Himanshu Madhani
Thanks,
- Himanshu
Re: [PATCH] scsi: lpfc: fix block guard enablement on SLI3 adapters
Martin, > Since f44ac12f1dcc, BG enablement is tracked with the > LPFC_SLI3_BG_ENABLED bit, which is set in lpfc_get_cfgparam before > lpfc_sli_config_sli_port() is called. The bit shouldn't be cleared > before checking the feature. Based on problem analysis by David Bond. Applied to 4.20/scsi-fixes, thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v3 3/4] target: add device vendor_id configfs attribute
On Wed, 2018-11-28 at 17:44 +0100, David Disseldorp wrote: > Hi Bart, > > On Wed, 28 Nov 2018 08:36:19 -0800, Bart Van Assche wrote: > > > Maybe I'm missing something, but why is zeroing of unused bytes in these > > functions > > necessary? Would the following be correct if all strings in struct t10_wwn > > would be > > '\0'-terminated? > > Your patch looks good to me. Mind if I tack it on to the end of my > t10_wwn.vendor/model/revision[size+1] patchset, with your authorship? Sure, that sounds fine to me. Bart.
