Re: [PATCH 01/10] scsi/mvsas: Kill CONFIG_SCSI_MVSAS_TASKLET

[John Garry <john.garry@xxxxxxxxxx>]

On 31/05/2022 00:15, Davidlohr Bueso wrote:
> Tasklets have long been deprecated as being too heavy on the system
> by running in irq context - and this is not a performance critical
> path. If a higher priority process wants to run, it must wait for
> the tasklet to finish before doing so. A more suitable equivalent
> is to converted to threaded irq instead.

/s/converted/convert/

> As such remove CONFIG_SCSI_MVSAS_TASKLET (which is horrible to begin
> with) and continue to do the async work, unconditionally now, just
> in task context. Just as with the tasklet version, device interrupts
> (MVS_IRQ_SAS_A/B) continues to be disabled from when the work was

/s/continues/continue/

> putted until it is actually complete. 

Question: Can there be any good reason to do this?

> Because there are no guarantees
> vs ksoftirqd, if this is broken for threaded irqs, then they are
> also broken for tasklets.
>
> Signed-off-by: Davidlohr Bueso <dave@xxxxxxxxxxxx>

Apart some comments/nits:

Reviewed-by: John Garry <john.garry@xxxxxxxxxx>

> ---
>   drivers/scsi/mvsas/Kconfig   |  7 ------
>   drivers/scsi/mvsas/mv_init.c | 44 ++++++------------------------------
>   drivers/scsi/mvsas/mv_sas.h  |  1 -
>   3 files changed, 7 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/scsi/mvsas/Kconfig b/drivers/scsi/mvsas/Kconfig
> index 79812b80743b..e9dd8dc84b1c 100644
> --- a/drivers/scsi/mvsas/Kconfig
> +++ b/drivers/scsi/mvsas/Kconfig
> @@ -23,10 +23,3 @@ config SCSI_MVSAS_DEBUG
>   	help
>   		Compiles the 88SE64XX/88SE94XX driver in debug mode.  In debug mode,
>   		the driver prints some messages to the console.
> -config SCSI_MVSAS_TASKLET
> -	bool "Support for interrupt tasklet"
> -	default n
> -	depends on SCSI_MVSAS
> -	help
> -		Compiles the 88SE64xx/88SE94xx driver in interrupt tasklet mode.In this mode,
> -		the interrupt will schedule a tasklet.
> diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
> index 2fde496fff5f..f36b270ba502 100644
> --- a/drivers/scsi/mvsas/mv_init.c
> +++ b/drivers/scsi/mvsas/mv_init.c
> @@ -146,12 +146,10 @@ static void mvs_free(struct mvs_info *mvi)
>   	kfree(mvi);
>   }
>   
> -#ifdef CONFIG_SCSI_MVSAS_TASKLET
> -static void mvs_tasklet(unsigned long opaque)
> +static irqreturn_t mvs_async(int irq, void *opaque)
>   {
>   	u32 stat;
>   	u16 core_nr, i = 0;
> -
>   	struct mvs_info *mvi;
>   	struct sas_ha_struct *sha = (struct sas_ha_struct *)opaque;

nit: you could have dropped this cast

>   
> @@ -172,46 +170,29 @@ static void mvs_tasklet(unsigned long opaque)
>   out:
>   	MVS_CHIP_DISP->interrupt_enable(mvi);
>   
> +	return IRQ_HANDLED;
>   }
> -#endif
>   
>   static irqreturn_t mvs_interrupt(int irq, void *opaque)
>   {
>   	u32 stat;
>   	struct mvs_info *mvi;
>   	struct sas_ha_struct *sha = opaque;
> -#ifndef CONFIG_SCSI_MVSAS_TASKLET
> -	u32 i;
> -	u32 core_nr;
> -
> -	core_nr = ((struct mvs_prv_info *)sha->lldd_ha)->n_host;
> -#endif
>   
>   	mvi = ((struct mvs_prv_info *)sha->lldd_ha)->mvi[0];
>   
>   	if (unlikely(!mvi))
>   		return IRQ_NONE;
> -#ifdef CONFIG_SCSI_MVSAS_TASKLET
> +
>   	MVS_CHIP_DISP->interrupt_disable(mvi);
> -#endif
>   
>   	stat = MVS_CHIP_DISP->isr_status(mvi, irq);
>   	if (!stat) {
> -	#ifdef CONFIG_SCSI_MVSAS_TASKLET
>   		MVS_CHIP_DISP->interrupt_enable(mvi);
> -	#endif
>   		return IRQ_NONE;
>   	}
>   
> -#ifdef CONFIG_SCSI_MVSAS_TASKLET
> -	tasklet_schedule(&((struct mvs_prv_info *)sha->lldd_ha)->mv_tasklet);
> -#else
> -	for (i = 0; i < core_nr; i++) {
> -		mvi = ((struct mvs_prv_info *)sha->lldd_ha)->mvi[i];
> -		MVS_CHIP_DISP->isr(mvi, irq, stat);
> -	}
> -#endif
> -	return IRQ_HANDLED;
> +	return IRQ_WAKE_THREAD;
>   }
>   
>   static int mvs_alloc(struct mvs_info *mvi, struct Scsi_Host *shost)
> @@ -557,14 +538,6 @@ static int mvs_pci_init(struct pci_dev *pdev, const struct pci_device_id *ent)
>   		}
>   		nhost++;
>   	} while (nhost < chip->n_host);
> -#ifdef CONFIG_SCSI_MVSAS_TASKLET
> -	{
> -	struct mvs_prv_info *mpi = SHOST_TO_SAS_HA(shost)->lldd_ha;
> -
> -	tasklet_init(&(mpi->mv_tasklet), mvs_tasklet,
> -		     (unsigned long)SHOST_TO_SAS_HA(shost));
> -	}
> -#endif
>   
>   	mvs_post_sas_ha_init(shost, chip);
>   
> @@ -575,8 +548,9 @@ static int mvs_pci_init(struct pci_dev *pdev, const struct pci_device_id *ent)
>   	rc = sas_register_ha(SHOST_TO_SAS_HA(shost));
>   	if (rc)
>   		goto err_out_shost;
> -	rc = request_irq(pdev->irq, irq_handler, IRQF_SHARED,
> -		DRV_NAME, SHOST_TO_SAS_HA(shost));
> +	rc = request_threaded_irq(pdev->irq, irq_handler, mvs_async,

any reason not to use devm version?

> +				  IRQF_SHARED, DRV_NAME,
> +				  SHOST_TO_SAS_HA(shost));
>   	if (rc)
>   		goto err_not_sas;
>   
> @@ -607,10 +581,6 @@ static void mvs_pci_remove(struct pci_dev *pdev)
>   	core_nr = ((struct mvs_prv_info *)sha->lldd_ha)->n_host;
>   	mvi = ((struct mvs_prv_info *)sha->lldd_ha)->mvi[0];
>   
> -#ifdef CONFIG_SCSI_MVSAS_TASKLET
> -	tasklet_kill(&((struct mvs_prv_info *)sha->lldd_ha)->mv_tasklet);
> -#endif
> -
>   	sas_unregister_ha(sha);
>   	sas_remove_host(mvi->shost);
>   
> diff --git a/drivers/scsi/mvsas/mv_sas.h b/drivers/scsi/mvsas/mv_sas.h
> index 509d8f32a04f..a0e87fdab98a 100644
> --- a/drivers/scsi/mvsas/mv_sas.h
> +++ b/drivers/scsi/mvsas/mv_sas.h
> @@ -403,7 +403,6 @@ struct mvs_prv_info{
>   	u8 scan_finished;
>   	u8 reserve;
>   	struct mvs_info *mvi[2];
> -	struct tasklet_struct mv_tasklet;
>   };
>   
>   struct mvs_wq {