diff mbox series

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

Message ID 20221214070608.4128546-3-yanaijie@huawei.com (mailing list archive)
State Superseded
Headers show
Series scsi: libsas: Some coding style fixes and cleanups | expand

Commit Message

Jason Yan Dec. 14, 2022, 7:06 a.m. UTC
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@oracle.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 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(-)

Comments

Jinpu Wang Dec. 14, 2022, 7:58 a.m. UTC | #1
On Wed, Dec 14, 2022 at 7:45 AM Jason Yan <yanaijie@huawei.com> 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@oracle.com>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
Reviewed-by: Jack Wang <jinpu.wang@ionos.com>
> ---
>  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")
>
>  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_ */
> --
> 2.31.1
>
John Garry Dec. 14, 2022, 10:01 a.m. UTC | #2
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@oracle.com>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>

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

> ---
>   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_ */
diff mbox series

Patch

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")
 
 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_ */