Re: [PATCH v5 5/5] target: perform t10_wwn ID initialisation in target_alloc_device()

[Bart Van Assche]

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(&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.

Agreed. I also would like to see that that if-condition is removed ...

Thanks,

Bart.

Re: [PATCH v5 5/5] target: perform t10_wwn ID initialisation in target_alloc_device()

[David Disseldorp]

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()

[David Disseldorp]

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

Re: [PATCH v5 5/5] target: perform t10_wwn ID initialisation in target_alloc_device()

[Hannes Reinecke]

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(&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

[PATCH v5 5/5] target: perform t10_wwn ID initialisation in target_alloc_device()

[David Disseldorp]

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(&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