Re: [PATCH v2 02/18] scsi: mptfusion: Simplify the alloc*_workqueue() invocations

[Damien Le Moal <dlemoal@kernel.org>]

On 8/17/24 06:55, Bart Van Assche wrote:
> Let alloc*_workqueue() format the workqueue names instead of calling
> snprintf() explicitly.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

In patch 1, you have all the changes for removing the use of
create_singlethread_workqueue() in a single patch, touching different drivers.
But the series has 17 more patches to further cleanup the workqueue API use in
various drivers. So why not have the changes in patch 1 split into these
different driver patches with a title like "Cleanup and simplify workqueue API
use" ? That would make reviewing easier I think and avoid having the patch 2-17
changing again code that was changed in patch 1...

> ---
>  drivers/message/fusion/mptbase.c | 10 +++-------
>  drivers/message/fusion/mptbase.h |  3 ---
>  drivers/message/fusion/mptfc.c   |  7 ++-----
>  3 files changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
> index 4bf669c55649..738bc4e60a18 100644
> --- a/drivers/message/fusion/mptbase.c
> +++ b/drivers/message/fusion/mptbase.c
> @@ -1856,10 +1856,8 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
>  	/* Initialize workqueue */
>  	INIT_DELAYED_WORK(&ioc->fault_reset_work, mpt_fault_reset_work);
>  
> -	snprintf(ioc->reset_work_q_name, MPT_KOBJ_NAME_LEN,
> -		 "mpt_poll_%d", ioc->id);
> -	ioc->reset_work_q = alloc_workqueue(ioc->reset_work_q_name,
> -					    WQ_MEM_RECLAIM, 0);
> +	ioc->reset_work_q =
> +		alloc_workqueue("mpt_poll_%d", WQ_MEM_RECLAIM, 0, ioc->id);
>  	if (!ioc->reset_work_q) {
>  		printk(MYIOC_s_ERR_FMT "Insufficient memory to add adapter!\n",
>  		    ioc->name);
> @@ -1986,9 +1984,7 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
>  
>  	INIT_LIST_HEAD(&ioc->fw_event_list);
>  	spin_lock_init(&ioc->fw_event_lock);
> -	snprintf(ioc->fw_event_q_name, MPT_KOBJ_NAME_LEN, "mpt/%d", ioc->id);
> -	ioc->fw_event_q = alloc_workqueue(ioc->fw_event_q_name,
> -					  WQ_MEM_RECLAIM, 0);
> +	ioc->fw_event_q = alloc_workqueue("mpt/%d", WQ_MEM_RECLAIM, 0, ioc->id);
>  	if (!ioc->fw_event_q) {
>  		printk(MYIOC_s_ERR_FMT "Insufficient memory to add adapter!\n",
>  		    ioc->name);
> diff --git a/drivers/message/fusion/mptbase.h b/drivers/message/fusion/mptbase.h
> index 8031173c3655..b406fd676da0 100644
> --- a/drivers/message/fusion/mptbase.h
> +++ b/drivers/message/fusion/mptbase.h
> @@ -729,7 +729,6 @@ typedef struct _MPT_ADAPTER
>  	struct list_head	 fw_event_list;
>  	spinlock_t		 fw_event_lock;
>  	u8			 fw_events_off; /* if '1', then ignore events */
> -	char 			 fw_event_q_name[MPT_KOBJ_NAME_LEN];
>  
>  	struct mutex		 sas_discovery_mutex;
>  	u8			 sas_discovery_runtime;
> @@ -764,7 +763,6 @@ typedef struct _MPT_ADAPTER
>  	u8			 fc_link_speed[2];
>  	spinlock_t		 fc_rescan_work_lock;
>  	struct work_struct	 fc_rescan_work;
> -	char			 fc_rescan_work_q_name[MPT_KOBJ_NAME_LEN];
>  	struct workqueue_struct *fc_rescan_work_q;
>  
>  	/* driver forced bus resets count */
> @@ -778,7 +776,6 @@ typedef struct _MPT_ADAPTER
>  	spinlock_t		  scsi_lookup_lock;
>  	u64			dma_mask;
>  	u32			  broadcast_aen_busy;
> -	char			 reset_work_q_name[MPT_KOBJ_NAME_LEN];
>  	struct workqueue_struct *reset_work_q;
>  	struct delayed_work	 fault_reset_work;
>  
> diff --git a/drivers/message/fusion/mptfc.c b/drivers/message/fusion/mptfc.c
> index a3c17c4fe69c..91242f26defb 100644
> --- a/drivers/message/fusion/mptfc.c
> +++ b/drivers/message/fusion/mptfc.c
> @@ -1349,11 +1349,8 @@ mptfc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  
>  	/* initialize workqueue */
>  
> -	snprintf(ioc->fc_rescan_work_q_name, sizeof(ioc->fc_rescan_work_q_name),
> -		 "mptfc_wq_%d", sh->host_no);
> -	ioc->fc_rescan_work_q =
> -		alloc_ordered_workqueue(ioc->fc_rescan_work_q_name,
> -					WQ_MEM_RECLAIM);
> +	ioc->fc_rescan_work_q = alloc_ordered_workqueue(
> +		"mptfc_wq_%d", WQ_MEM_RECLAIM, sh->host_no);
>  	if (!ioc->fc_rescan_work_q) {
>  		error = -ENOMEM;
>  		goto out_mptfc_host;
> 

-- 
Damien Le Moal
Western Digital Research