mbox series

[v2,0/7] target: make tpg/enable attribute

Message ID 20210322080554.7611-1-d.bogdanov@yadro.com
Headers show
Series target: make tpg/enable attribute | expand

Message

Dmitry Bogdanov March 22, 2021, 8:05 a.m. UTC
Many fabric modules provide their own implementation of enable
attribute in tpg.
The change set removes the code duplication and automatically adds "enable"
attribute for fabric modules that has an implementation of
fabric_enable_tpg() ops.

This patchset is intended for scsi-queue.

v2:
    create enable atribute only for modules with enable_tpg ops
    add patches for srpt, usb and ibmvscsi

Dmitry Bogdanov (7):
  target: core: add common tpg/enable attribute
  target: iscsi: replace enable attr to ops.enable
  target: qla2xx: replace enable attr to ops.enable
  target: sbp: replace enable attr to ops.enable
  target: srpt replace enable attr to ops.enable
  target: ibm_vscsi: replace enable attr to ops.enable
  target: usb: replace enable attr to ops.enable

 drivers/infiniband/ulp/srpt/ib_srpt.c        | 38 +-------
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c     | 42 +--------
 drivers/scsi/qla2xxx/tcm_qla2xxx.c           | 73 +++-------------
 drivers/target/iscsi/iscsi_target_configfs.c | 91 +++++++-------------
 drivers/target/sbp/sbp_target.c              | 30 ++-----
 drivers/target/target_core_configfs.c        |  1 +
 drivers/target/target_core_fabric_configfs.c | 40 ++++++++-
 drivers/target/target_core_internal.h        |  1 +
 drivers/target/target_core_tpg.c             | 41 +++++++++
 drivers/usb/gadget/function/f_tcm.c          | 31 ++-----
 include/target/target_core_base.h            |  1 +
 include/target/target_core_fabric.h          |  1 +
 12 files changed, 146 insertions(+), 244 deletions(-)

Comments

Himanshu Madhani March 22, 2021, 2:49 p.m. UTC | #1
> On Mar 22, 2021, at 3:05 AM, Dmitry Bogdanov <d.bogdanov@yadro.com> wrote:
> 
> Many fabric modules provide their own implementation of enable
> attribute in tpg.
> The change set removes the code duplication and automatically adds "enable"
> attribute for fabric modules that has an implementation of
> fabric_enable_tpg() ops.
> 
> This patchset is intended for scsi-queue.
> 
> v2:
>    create enable atribute only for modules with enable_tpg ops
>    add patches for srpt, usb and ibmvscsi
> 
> Dmitry Bogdanov (7):
>  target: core: add common tpg/enable attribute
>  target: iscsi: replace enable attr to ops.enable
>  target: qla2xx: replace enable attr to ops.enable
>  target: sbp: replace enable attr to ops.enable
>  target: srpt replace enable attr to ops.enable
>  target: ibm_vscsi: replace enable attr to ops.enable
>  target: usb: replace enable attr to ops.enable
> 
> drivers/infiniband/ulp/srpt/ib_srpt.c        | 38 +-------
> drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c     | 42 +--------
> drivers/scsi/qla2xxx/tcm_qla2xxx.c           | 73 +++-------------
> drivers/target/iscsi/iscsi_target_configfs.c | 91 +++++++-------------
> drivers/target/sbp/sbp_target.c              | 30 ++-----
> drivers/target/target_core_configfs.c        |  1 +
> drivers/target/target_core_fabric_configfs.c | 40 ++++++++-
> drivers/target/target_core_internal.h        |  1 +
> drivers/target/target_core_tpg.c             | 41 +++++++++
> drivers/usb/gadget/function/f_tcm.c          | 31 ++-----
> include/target/target_core_base.h            |  1 +
> include/target/target_core_fabric.h          |  1 +
> 12 files changed, 146 insertions(+), 244 deletions(-)
> 
> -- 
> 2.25.1
> 

Series looks good.

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>

--
Himanshu Madhani	 Oracle Linux Engineering
Mike Christie March 22, 2021, 5:43 p.m. UTC | #2
On 3/22/21 3:05 AM, Dmitry Bogdanov wrote:
>  
> +static int lio_target_tiqn_enabletpg(struct se_portal_group *se_tpg,
> +					 bool enable)
> +{
This driver uses 2 or 3 coding styles for this, but the above is a new one.
Could you tab over then use spaces to line with with "(" so we limit the
styles?
Konstantin Shelekhin March 24, 2021, 9:51 p.m. UTC | #3
On Mon, Mar 22, 2021 at 11:05:48AM +0300, Dmitry Bogdanov wrote:
> +static int

> +target_fabric_setup_tpg_base_cit(struct target_fabric_configfs *tf)

> +{

> +	int i, k, len = 0;

> +	struct config_item_type *cit = &tf->tf_tpg_base_cit;

> +	struct configfs_attribute **attrs;

> +

> +	if (tf->tf_ops->fabric_enable_tpg)

> +		for (i = 0; core_tpg_base_enable_attrs[i]; i++)

> +			len += sizeof(struct configfs_attribute *);

...
> +	if (tf->tf_ops->fabric_enable_tpg)

> +		for (i = 0; core_tpg_base_enable_attrs[i]; i++)

...
>+CONFIGFS_ATTR(core_tpg_base_, enable);

>+

>+struct configfs_attribute *core_tpg_base_enable_attrs[] = {

>+       &core_tpg_base_attr_enable,

>+       NULL,


I believe that there is no real benefit in core_tpg_base_enable_attrs[]
with only one attribute. Just use core_tpg_base_attr_enable directly.