Re: [PATCH v3 2/5] scsi: libsas: change the coding style of sas_discover_sata()

[John Garry <john.g.garry@xxxxxxxxxx>]

On 14/12/2022 07:06, Jason Yan wrote:
> The coding style where calling this interface is inconsistent with other
> interfaces for SATA devices. The standard style for other SATA interfaces
> is like:
>
>      #ifdefine CONFIG_SCSI_SAS_ATA
>      void sas_ata_task_abort(struct sas_task *task);
>      #else
>      static inline void sas_ata_task_abort(struct sas_task *task)
>      {
>      }
>      #endif
>
> And the callers does not have to do things like "#ifdefine CONFIG_SCSI_SAS_ATA"
> and may call the interface directly. So follow the standard style here.
>
> Cc: John Garry <john.g.garry@xxxxxxxxxx>
> Signed-off-by: Jason Yan <yanaijie@xxxxxxxxxx>

Regardless of comment on the new macro, below:
Reviewed-by: John Garry <john.g.garry@xxxxxxxxxx>

> ---
>   drivers/scsi/libsas/sas_discover.c | 6 ------
>   include/scsi/libsas.h              | 1 -
>   include/scsi/sas_ata.h             | 9 +++++++++
>   3 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
> index d5bc1314c341..72fdb2e5d047 100644
> --- a/drivers/scsi/libsas/sas_discover.c
> +++ b/drivers/scsi/libsas/sas_discover.c
> @@ -455,14 +455,8 @@ static void sas_discover_domain(struct work_struct *work)
>   		break;
>   	case SAS_SATA_DEV:
>   	case SAS_SATA_PM:
> -#ifdef CONFIG_SCSI_SAS_ATA
>   		error = sas_discover_sata(dev);
>   		break;
> -#else
> -		pr_notice("ATA device seen but CONFIG_SCSI_SAS_ATA=N so cannot attach\n");
> -		fallthrough;
> -#endif
> -		/* Fall through - only for the #else condition above. */
>   	default:
>   		error = -ENXIO;
>   		pr_err("unhandled device %d\n", dev->dev_type);
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index 1aee3d0ebbb2..159823e0afbf 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -735,7 +735,6 @@ void sas_unregister_domain_devices(struct asd_sas_port *port, int gone);
>   void sas_init_disc(struct sas_discovery *disc, struct asd_sas_port *);
>   void sas_discover_event(struct asd_sas_port *, enum discover_event ev);
>   
> -int  sas_discover_sata(struct domain_device *);
>   int  sas_discover_end_dev(struct domain_device *);
>   
>   void sas_unregister_dev(struct asd_sas_port *port, struct domain_device *);
> diff --git a/include/scsi/sas_ata.h b/include/scsi/sas_ata.h
> index 9c927d46f136..7cdba456b746 100644
> --- a/include/scsi/sas_ata.h
> +++ b/include/scsi/sas_ata.h
> @@ -36,8 +36,11 @@ void sas_ata_device_link_abort(struct domain_device *dev, bool force_reset);
>   int sas_execute_ata_cmd(struct domain_device *device, u8 *fis,
>   			int force_phy_id);
>   int smp_ata_check_ready_type(struct ata_link *link);
> +int sas_discover_sata(struct domain_device *dev);
>   #else
>   
> +#define SAS_ATA_DISABLED_NOTICE \
> +	pr_notice_once("ATA device seen but CONFIG_SCSI_SAS_ATA=N\n")

Personally I would prefer a function and not a macro, like:

void sas_ata_disabled_notice(void)
{
	pr_notice_once("ATA device seen but CONFIG_SCSI_SAS_ATA=N\n");
}

And even if you stick with the macro, I think that we normally would 
include the ',' in the macro

>   
>   static inline int dev_is_sata(struct domain_device *dev)
>   {
> @@ -103,6 +106,12 @@ static inline int smp_ata_check_ready_type(struct ata_link *link)
>   {
>   	return 0;
>   }
> +
> +static inline int sas_discover_sata(struct domain_device *dev)
> +{
> +	SAS_ATA_DISABLED_NOTICE;
> +	return -ENXIO;
> +}
>   #endif
>   
>   #endif /* _SAS_ATA_H_ */