RE: [PATCH RFC v7 10/12] megaraid_sas: switch fusion adapters to MQ

From: Kashyap Desai <kashyap.desai@broadcom.com>
To: John Garry <john.garry@huawei.com>,
	axboe@kernel.dk, jejb@linux.ibm.com, martin.petersen@oracle.com,
	don.brace@microsemi.com, Sumit Saxena <sumit.saxena@broadcom.com>,
	ming.lei@redhat.com, bvanassche@acm.org, hare@suse.com,
	hch@lst.de,
	Shivasharan Srikanteshwara 
	<shivasharan.srikanteshwara@broadcom.com>
Cc: linux-block@vger.kernel.org, linux-scsi@vger.kernel.org,
	esc.storagedev@microsemi.com, chenxiang66@hisilicon.com,
	"PDL,MEGARAIDLINUX" <megaraidlinux.pdl@broadcom.com>
Subject: RE: [PATCH RFC v7 10/12] megaraid_sas: switch fusion adapters to MQ
Date: Tue, 7 Jul 2020 20:15:34 +0530	[thread overview]
Message-ID: <0563e53f843c97de1a5a035fae892bf8@mail.gmail.com> (raw)
In-Reply-To: <10d36c09-9d5b-92e9-23ac-ea1a2628e7d9@huawei.com>

> >>>
> >>>    #include <scsi/scsi.h>
> >>>    #include <scsi/scsi_cmnd.h>
> >>> @@ -113,6 +114,10 @@ unsigned int enable_sdev_max_qd;
> >>>    module_param(enable_sdev_max_qd, int, 0444);
> >>>    MODULE_PARM_DESC(enable_sdev_max_qd, "Enable sdev max qd as
> >> can_queue.
> >>> Default: 0");
> >>>
> >>> +int host_tagset_disabled = 0;
> >>> +module_param(host_tagset_disabled, int, 0444);
> >>> +MODULE_PARM_DESC(host_tagset_disabled, "Shared host tagset
> >> enable/disable
> >>> Default: enable(1)");
> >> The logic seems inverted here: for passing 1, I would expect Shared
> >> host tagset enabled, while it actually means to disable, right?
> > No. passing 1 means shared_hosttag support will be turned off.
>
> Just reading "Shared host tagset enable/disable Default: enable(1)"
> looked inconsistent to me.

I will change to "host_tagset_enable" that will be good for readability.
Default value will of host_tagset_enable will be 1 and user can turnoff
passing 0.

>
> >>> +
> >>>    MODULE_LICENSE("GPL");
> >>>    MODULE_VERSION(MEGASAS_VERSION);
> >>>    MODULE_AUTHOR("megaraidlinux.pdl@broadcom.com");
> >>> @@ -3115,6 +3120,18 @@ megasas_bios_param(struct scsi_device
> *sdev,
> >> struct
> >>> block_device *bdev,
> >>>           return 0;
> >>>    }
> >>>
> >>> +static int megasas_map_queues(struct Scsi_Host *shost) {
> >>> +       struct megasas_instance *instance;
> >>> +       instance = (struct megasas_instance *)shost->hostdata;
> >>> +
> >>> +       if (instance->host->nr_hw_queues == 1)
> >>> +               return 0;
> >>> +
> >>> +       return
> >>> blk_mq_pci_map_queues(&shost->tag_set.map[HCTX_TYPE_DEFAULT],
> >>> +                       instance->pdev,
> >>> instance->low_latency_index_start);
> >>> +}
> >>> +
> >>>    static void megasas_aen_polling(struct work_struct *work);
> >>>
> >>>    /**
> >>> @@ -3423,8 +3440,10 @@ static struct scsi_host_template
> >> megasas_template =
> >>> {
> >>>           .eh_timed_out = megasas_reset_timer,
> >>>           .shost_attrs = megaraid_host_attrs,
> >>>           .bios_param = megasas_bios_param,
> >>> +       .map_queues = megasas_map_queues,
> >>>           .change_queue_depth = scsi_change_queue_depth,
> >>>           .max_segment_size = 0xffffffff,
> >>> +       .host_tagset = 1,
> >> Is your intention to always have this set for Scsi_Host, and just
> >> change nr_hw_queues?
> > Actually I wanted to turn off  this feature using host_tagset and not
> > through nr_hw_queue. I will address this.
> >
> > Additional request -
> > In MR we have old controllers (called MFI_SERIES). We prefer not to
> > change behavior for those controller.
> > Having host_tagset in template does not allow to cherry pick different
> > values for different type of controller.
>
> Ok, so it seems sensible to add host_tagset to Scsi_Host structure also,
> to
> allow overwriting during probe time.
>
> If you want to share an updated megaraid sas driver patch based on that,
> then
> that's fine. I can incorporate that change in the patch where we add
> host_tagset to the scsi host template.

If you share git repo link of next submission, I can send you megaraid_sas
driver patch which you can include in series.

Kashyap
>
> > If host_tagset is part of Scsi_Host OR we add check in scsi_lib.c that
> > host_tagset = 1 only make sense if nr_hw_queues > 1, we can cherry
> > pick in driver.
> >
> >
>
>