* [PATCH v5 5/5] target: perform t10_wwn ID initialisation in target_alloc_device()
@ 2018-11-30 23:34 David Disseldorp
2018-12-01 14:59 ` Hannes Reinecke
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: David Disseldorp @ 2018-11-30 23:34 UTC (permalink / raw)
To: target-devel
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 <ddiss@suse.de>
---
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(&xcopy_lun->lun_tg_pt_md_mutex);
xcopy_lun->lun_tpg = &xcopy_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;
}
@@ -984,23 +1001,6 @@ int target_configure_device(struct se_device *dev)
*/
INIT_WORK(&dev->qf_work_queue, target_qf_do_work);
- /*
- * 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));
- }
-
scsi_dump_inquiry(dev);
spin_lock(&hba->device_lock);
--
2.13.7
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v5 5/5] target: perform t10_wwn ID initialisation in target_alloc_device()
2018-11-30 23:34 [PATCH v5 5/5] target: perform t10_wwn ID initialisation in target_alloc_device() David Disseldorp
@ 2018-12-01 14:59 ` Hannes Reinecke
2018-12-02 22:22 ` David Disseldorp
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Hannes Reinecke @ 2018-12-01 14:59 UTC (permalink / raw)
To: target-devel
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 <ddiss@suse.de>
> ---
> 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(&xcopy_lun->lun_tg_pt_md_mutex);
> xcopy_lun->lun_tpg = &xcopy_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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v5 5/5] target: perform t10_wwn ID initialisation in target_alloc_device()
2018-11-30 23:34 [PATCH v5 5/5] target: perform t10_wwn ID initialisation in target_alloc_device() David Disseldorp
2018-12-01 14:59 ` Hannes Reinecke
@ 2018-12-02 22:22 ` David Disseldorp
2018-12-03 11:52 ` David Disseldorp
2018-12-04 0:35 ` Bart Van Assche
3 siblings, 0 replies; 5+ messages in thread
From: David Disseldorp @ 2018-12-02 22:22 UTC (permalink / raw)
To: target-devel
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(&xcopy_lun->lun_tg_pt_md_mutex);
> > xcopy_lun->lun_tpg = &xcopy_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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v5 5/5] target: perform t10_wwn ID initialisation in target_alloc_device()
2018-11-30 23:34 [PATCH v5 5/5] target: perform t10_wwn ID initialisation in target_alloc_device() David Disseldorp
2018-12-01 14:59 ` Hannes Reinecke
2018-12-02 22:22 ` David Disseldorp
@ 2018-12-03 11:52 ` David Disseldorp
2018-12-04 0:35 ` Bart Van Assche
3 siblings, 0 replies; 5+ messages in thread
From: David Disseldorp @ 2018-12-03 11:52 UTC (permalink / raw)
To: target-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v5 5/5] target: perform t10_wwn ID initialisation in target_alloc_device()
2018-11-30 23:34 [PATCH v5 5/5] target: perform t10_wwn ID initialisation in target_alloc_device() David Disseldorp
` (2 preceding siblings ...)
2018-12-03 11:52 ` David Disseldorp
@ 2018-12-04 0:35 ` Bart Van Assche
3 siblings, 0 replies; 5+ messages in thread
From: Bart Van Assche @ 2018-12-04 0:35 UTC (permalink / raw)
To: target-devel
On Sat, 2018-12-01 at 15:59 +-0100, Hannes Reinecke wrote:
+AD4 On 12/1/18 12:34 AM, David Disseldorp wrote:
+AD4 +AD4 Initialise the t10+AF8-wwn vendor, model and revision defaults when a
+AD4 +AD4 device is allocated instead of when it's enabled. This ensures that
+AD4 +AD4 custom vendor or model strings set prior to enablement are not later
+AD4 +AD4 overwritten with default values.
+AD4 +AD4
+AD4 +AD4 Signed-off-by: David Disseldorp +ADw-ddiss+AEA-suse.de+AD4
+AD4 +AD4 ---
+AD4 +AD4 drivers/target/target+AF8-core+AF8-device.c +AHw 34 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+------------------
+AD4 +AD4 1 file changed, 17 insertions(+-), 17 deletions(-)
+AD4 +AD4 +AD4 diff --git a/drivers/target/target+AF8-core+AF8-device.c
+AD4
+AD4 b/drivers/target/target+AF8-core+AF8-device.c
+AD4 +AD4 index 5512871f50e4..6318d59a1564 100644
+AD4 +AD4 --- a/drivers/target/target+AF8-core+AF8-device.c
+AD4 +AD4 +-+-+- b/drivers/target/target+AF8-core+AF8-device.c
+AD4 +AD4 +AEAAQA -810,6 +-810,23 +AEAAQA struct se+AF8-device +ACo-target+AF8-alloc+AF8-device(struct se+AF8-hba +ACo-hba, const char +ACo-name)
+AD4 +AD4 mutex+AF8-init(+ACY-xcopy+AF8-lun-+AD4-lun+AF8-tg+AF8-pt+AF8-md+AF8-mutex)+ADs
+AD4 +AD4 xcopy+AF8-lun-+AD4-lun+AF8-tpg +AD0 +ACY-xcopy+AF8-pt+AF8-tpg+ADs
+AD4 +AD4
+AD4 +AD4 +- /+ACo
+AD4 +AD4 +- +ACo Preload the initial INQUIRY const values if we are doing
+AD4 +AD4 +- +ACo anything virtual (IBLOCK, FILEIO, RAMDISK), but not for TCM/pSCSI
+AD4 +AD4 +- +ACo passthrough because this is being provided by the backend LLD.
+AD4 +AD4 +- +ACo-/
+AD4 +AD4 +- BUILD+AF8-BUG+AF8-ON(sizeof(dev-+AD4-t10+AF8-wwn.vendor) +ACEAPQ INQUIRY+AF8-VENDOR+AF8-LEN +- 1)+ADs
+AD4 +AD4 +- BUILD+AF8-BUG+AF8-ON(sizeof(dev-+AD4-t10+AF8-wwn.model) +ACEAPQ INQUIRY+AF8-MODEL+AF8-LEN +- 1)+ADs
+AD4 +AD4 +- BUILD+AF8-BUG+AF8-ON(sizeof(dev-+AD4-t10+AF8-wwn.revision) +ACEAPQ INQUIRY+AF8-REVISION+AF8-LEN +- 1)+ADs
+AD4 +AD4 +- if (+ACE(dev-+AD4-transport-+AD4-transport+AF8-flags +ACY TRANSPORT+AF8-FLAG+AF8-PASSTHROUGH)) +AHs
+AD4 +AD4 +- strlcpy(dev-+AD4-t10+AF8-wwn.vendor, +ACI-LIO-ORG+ACI,
+AD4 +AD4 +- sizeof(dev-+AD4-t10+AF8-wwn.vendor))+ADs
+AD4 +AD4 +- strlcpy(dev-+AD4-t10+AF8-wwn.model, dev-+AD4-transport-+AD4-inquiry+AF8-prod,
+AD4 +AD4 +- sizeof(dev-+AD4-t10+AF8-wwn.model))+ADs
+AD4 +AD4 +- strlcpy(dev-+AD4-t10+AF8-wwn.revision, dev-+AD4-transport-+AD4-inquiry+AF8-rev,
+AD4 +AD4 +- sizeof(dev-+AD4-t10+AF8-wwn.revision))+ADs
+AD4 +AD4 +- +AH0
+AD4 +AD4 +-
+AD4 +AD4 return dev+ADs
+AD4 +AD4 +AH0
+AD4 +AD4
+AD4
+AD4 This is odd. I'd rather have it consistent across backends, ie either
+AD4 move the initialisation into the backends, or provide a means to check
+AD4 if the inquiry data has already been pre-filled.
+AD4 But this check really is awkward.
Agreed. I also would like to see that that if-condition is removed ...
Thanks,
Bart.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-12-04 0:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-30 23:34 [PATCH v5 5/5] target: perform t10_wwn ID initialisation in target_alloc_device() David Disseldorp
2018-12-01 14:59 ` Hannes Reinecke
2018-12-02 22:22 ` David Disseldorp
2018-12-03 11:52 ` David Disseldorp
2018-12-04 0:35 ` Bart Van Assche
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).