[PATCH] scsi: sas: scsi_queue_work can fail, so make callers aware

[PATCH] scsi: sas: scsi_queue_work can fail, so make callers aware

[Johannes Thumshirn]

libsas uses scsi_queue_work() to queue it's internal event
notifications. scsi_queue_work() can return -EINVAL if the work queue
doesn't exist and it does call queue_work() which can return false if
the work is already queued.

Make the SAS event code capable of returning errors up to the caller,
which is handy when changing to dynamically allocated work in libsas
as well, as discussed here: https://lkml.org/lkml/2017/6/14/121.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/libsas/sas_event.c    | 36 ++++++++++++++++++++++--------------
 drivers/scsi/libsas/sas_internal.h |  4 ++--
 include/scsi/libsas.h              |  6 +++---
 3 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
index aadbd5314c5c..c0d0d979b76d 100644
--- a/drivers/scsi/libsas/sas_event.c
+++ b/drivers/scsi/libsas/sas_event.c
@@ -27,30 +27,38 @@
 #include "sas_internal.h"
 #include "sas_dump.h"
 
-void sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw)
+int sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw)
 {
+	int rc = 0;
+
 	if (!test_bit(SAS_HA_REGISTERED, &ha->state))
-		return;
+		return 0;
 
 	if (test_bit(SAS_HA_DRAINING, &ha->state)) {
 		/* add it to the defer list, if not already pending */
 		if (list_empty(&sw->drain_node))
 			list_add(&sw->drain_node, &ha->defer_q);
 	} else
-		scsi_queue_work(ha->core.shost, &sw->work);
+		rc = scsi_queue_work(ha->core.shost, &sw->work);
+
+	return rc;
 }
 
-static void sas_queue_event(int event, unsigned long *pending,
+static int sas_queue_event(int event, unsigned long *pending,
 			    struct sas_work *work,
 			    struct sas_ha_struct *ha)
 {
+	int rc = 0;
+
 	if (!test_and_set_bit(event, pending)) {
 		unsigned long flags;
 
 		spin_lock_irqsave(&ha->lock, flags);
-		sas_queue_work(ha, work);
+		rc = sas_queue_work(ha, work);
 		spin_unlock_irqrestore(&ha->lock, flags);
 	}
+
+	return rc;
 }
 
 
@@ -116,32 +124,32 @@ void sas_enable_revalidation(struct sas_ha_struct *ha)
 	mutex_unlock(&ha->disco_mutex);
 }
 
-static void notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event event)
+static int notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event event)
 {
 	BUG_ON(event >= HA_NUM_EVENTS);
 
-	sas_queue_event(event, &sas_ha->pending,
-			&sas_ha->ha_events[event].work, sas_ha);
+	return sas_queue_event(event, &sas_ha->pending,
+			       &sas_ha->ha_events[event].work, sas_ha);
 }
 
-static void notify_port_event(struct asd_sas_phy *phy, enum port_event event)
+static int notify_port_event(struct asd_sas_phy *phy, enum port_event event)
 {
 	struct sas_ha_struct *ha = phy->ha;
 
 	BUG_ON(event >= PORT_NUM_EVENTS);
 
-	sas_queue_event(event, &phy->port_events_pending,
-			&phy->port_events[event].work, ha);
+	return sas_queue_event(event, &phy->port_events_pending,
+			       &phy->port_events[event].work, ha);
 }
 
-void sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event)
+int sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event)
 {
 	struct sas_ha_struct *ha = phy->ha;
 
 	BUG_ON(event >= PHY_NUM_EVENTS);
 
-	sas_queue_event(event, &phy->phy_events_pending,
-			&phy->phy_events[event].work, ha);
+	return sas_queue_event(event, &phy->phy_events_pending,
+			       &phy->phy_events[event].work, ha);
 }
 
 int sas_init_events(struct sas_ha_struct *sas_ha)
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index b306b7843d99..a216c957b639 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -76,7 +76,7 @@ void sas_porte_broadcast_rcvd(struct work_struct *work);
 void sas_porte_link_reset_err(struct work_struct *work);
 void sas_porte_timer_event(struct work_struct *work);
 void sas_porte_hard_reset(struct work_struct *work);
-void sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw);
+int sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw);
 
 int sas_notify_lldd_dev_found(struct domain_device *);
 void sas_notify_lldd_dev_gone(struct domain_device *);
@@ -85,7 +85,7 @@ int sas_smp_phy_control(struct domain_device *dev, int phy_id,
 			enum phy_func phy_func, struct sas_phy_linkrates *);
 int sas_smp_get_phy_events(struct sas_phy *phy);
 
-void sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event);
+int sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event);
 void sas_device_set_phy(struct domain_device *dev, struct sas_port *port);
 struct domain_device *sas_find_dev_by_rphy(struct sas_rphy *rphy);
 struct domain_device *sas_ex_to_ata(struct domain_device *ex_dev, int phy_id);
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index dd0f72c95abe..cfaeed256ab2 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -415,9 +415,9 @@ struct sas_ha_struct {
 				* their siblings when forming wide ports */
 
 	/* LLDD calls these to notify the class of an event. */
-	void (*notify_ha_event)(struct sas_ha_struct *, enum ha_event);
-	void (*notify_port_event)(struct asd_sas_phy *, enum port_event);
-	void (*notify_phy_event)(struct asd_sas_phy *, enum phy_event);
+	int (*notify_ha_event)(struct sas_ha_struct *, enum ha_event);
+	int (*notify_port_event)(struct asd_sas_phy *, enum port_event);
+	int (*notify_phy_event)(struct asd_sas_phy *, enum phy_event);
 
 	void *lldd_ha;		  /* not touched by sas class code */
 
-- 
2.12.3

Re: [PATCH] scsi: sas: scsi_queue_work can fail, so make callers aware

[John Garry]

On 14/06/2017 12:52, Johannes Thumshirn wrote:
> libsas uses scsi_queue_work() to queue it's internal event
> notifications. scsi_queue_work() can return -EINVAL if the work queue
> doesn't exist and it does call queue_work() which can return false if
> the work is already queued.
>

Hi Johannes,

When the queuing fails and we an report error, what is the caller, i.e. 
the LLDD, supposed to do with this information?

I mean, the LLDD is just reporting an event, like a broadcast event, and 
libsas could not handle it for some reason. The LLDD probably does not 
know how to handle this, apart from printing an error or maybe even 
disabling the originating PHY.

> Make the SAS event code capable of returning errors up to the caller,
> which is handy when changing to dynamically allocated work in libsas
> as well, as discussed here: https://lkml.org/lkml/2017/6/14/121.

I will comment on this patchset separately.

Much appreciated,
John

>
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  drivers/scsi/libsas/sas_event.c    | 36 ++++++++++++++++++++++--------------
>  drivers/scsi/libsas/sas_internal.h |  4 ++--
>  include/scsi/libsas.h              |  6 +++---
>  3 files changed, 27 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
> index aadbd5314c5c..c0d0d979b76d 100644
> --- a/drivers/scsi/libsas/sas_event.c
> +++ b/drivers/scsi/libsas/sas_event.c
> @@ -27,30 +27,38 @@
>  #include "sas_internal.h"
>  #include "sas_dump.h"
>
> -void sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw)
> +int sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw)
>  {
> +	int rc = 0;
> +
>  	if (!test_bit(SAS_HA_REGISTERED, &ha->state))
> -		return;
> +		return 0;
>
>  	if (test_bit(SAS_HA_DRAINING, &ha->state)) {
>  		/* add it to the defer list, if not already pending */
>  		if (list_empty(&sw->drain_node))
>  			list_add(&sw->drain_node, &ha->defer_q);
>  	} else
> -		scsi_queue_work(ha->core.shost, &sw->work);
> +		rc = scsi_queue_work(ha->core.shost, &sw->work);
> +
> +	return rc;
>  }
>
> -static void sas_queue_event(int event, unsigned long *pending,
> +static int sas_queue_event(int event, unsigned long *pending,
>  			    struct sas_work *work,
>  			    struct sas_ha_struct *ha)
>  {
> +	int rc = 0;
> +
>  	if (!test_and_set_bit(event, pending)) {
>  		unsigned long flags;
>
>  		spin_lock_irqsave(&ha->lock, flags);
> -		sas_queue_work(ha, work);
> +		rc = sas_queue_work(ha, work);
>  		spin_unlock_irqrestore(&ha->lock, flags);
>  	}
> +
> +	return rc;
>  }
>
>
> @@ -116,32 +124,32 @@ void sas_enable_revalidation(struct sas_ha_struct *ha)
>  	mutex_unlock(&ha->disco_mutex);
>  }
>
> -static void notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event event)
> +static int notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event event)
>  {
>  	BUG_ON(event >= HA_NUM_EVENTS);
>
> -	sas_queue_event(event, &sas_ha->pending,
> -			&sas_ha->ha_events[event].work, sas_ha);
> +	return sas_queue_event(event, &sas_ha->pending,
> +			       &sas_ha->ha_events[event].work, sas_ha);
>  }
>
> -static void notify_port_event(struct asd_sas_phy *phy, enum port_event event)
> +static int notify_port_event(struct asd_sas_phy *phy, enum port_event event)
>  {
>  	struct sas_ha_struct *ha = phy->ha;
>
>  	BUG_ON(event >= PORT_NUM_EVENTS);
>
> -	sas_queue_event(event, &phy->port_events_pending,
> -			&phy->port_events[event].work, ha);
> +	return sas_queue_event(event, &phy->port_events_pending,
> +			       &phy->port_events[event].work, ha);
>  }
>
> -void sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event)
> +int sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event)
>  {
>  	struct sas_ha_struct *ha = phy->ha;
>
>  	BUG_ON(event >= PHY_NUM_EVENTS);
>
> -	sas_queue_event(event, &phy->phy_events_pending,
> -			&phy->phy_events[event].work, ha);
> +	return sas_queue_event(event, &phy->phy_events_pending,
> +			       &phy->phy_events[event].work, ha);
>  }
>
>  int sas_init_events(struct sas_ha_struct *sas_ha)
> diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
> index b306b7843d99..a216c957b639 100644
> --- a/drivers/scsi/libsas/sas_internal.h
> +++ b/drivers/scsi/libsas/sas_internal.h
> @@ -76,7 +76,7 @@ void sas_porte_broadcast_rcvd(struct work_struct *work);
>  void sas_porte_link_reset_err(struct work_struct *work);
>  void sas_porte_timer_event(struct work_struct *work);
>  void sas_porte_hard_reset(struct work_struct *work);
> -void sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw);
> +int sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw);
>
>  int sas_notify_lldd_dev_found(struct domain_device *);
>  void sas_notify_lldd_dev_gone(struct domain_device *);
> @@ -85,7 +85,7 @@ int sas_smp_phy_control(struct domain_device *dev, int phy_id,
>  			enum phy_func phy_func, struct sas_phy_linkrates *);
>  int sas_smp_get_phy_events(struct sas_phy *phy);
>
> -void sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event);
> +int sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event);
>  void sas_device_set_phy(struct domain_device *dev, struct sas_port *port);
>  struct domain_device *sas_find_dev_by_rphy(struct sas_rphy *rphy);
>  struct domain_device *sas_ex_to_ata(struct domain_device *ex_dev, int phy_id);
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index dd0f72c95abe..cfaeed256ab2 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -415,9 +415,9 @@ struct sas_ha_struct {
>  				* their siblings when forming wide ports */
>
>  	/* LLDD calls these to notify the class of an event. */
> -	void (*notify_ha_event)(struct sas_ha_struct *, enum ha_event);
> -	void (*notify_port_event)(struct asd_sas_phy *, enum port_event);
> -	void (*notify_phy_event)(struct asd_sas_phy *, enum phy_event);
> +	int (*notify_ha_event)(struct sas_ha_struct *, enum ha_event);
> +	int (*notify_port_event)(struct asd_sas_phy *, enum port_event);
> +	int (*notify_phy_event)(struct asd_sas_phy *, enum phy_event);
>
>  	void *lldd_ha;		  /* not touched by sas class code */
>
>

Re: [PATCH] scsi: sas: scsi_queue_work can fail, so make callers aware

[Johannes Thumshirn]

On 06/14/2017 02:57 PM, John Garry wrote:
> On 14/06/2017 12:52, Johannes Thumshirn wrote:
>> libsas uses scsi_queue_work() to queue it's internal event
>> notifications. scsi_queue_work() can return -EINVAL if the work queue
>> doesn't exist and it does call queue_work() which can return false if
>> the work is already queued.
>>
> 
> Hi Johannes,
> 
> When the queuing fails and we an report error, what is the caller, i.e.
> the LLDD, supposed to do with this information?
> 
> I mean, the LLDD is just reporting an event, like a broadcast event, and
> libsas could not handle it for some reason. The LLDD probably does not
> know how to handle this, apart from printing an error or maybe even
> disabling the originating PHY.

This depends on the event and the LLDD I guess. If it's -EINVAL (a.k.a
SCSI work queue not present) there's not much an LLDD can do about. If
it's a plain false, we could for instance report that we re-queue an
event or something similar.

>> Make the SAS event code capable of returning errors up to the caller,
>> which is handy when changing to dynamically allocated work in libsas
>> as well, as discussed here: https://lkml.org/lkml/2017/6/14/121.
> 
> I will comment on this patchset separately.

It's merrily a preparation patch for this libsas patchset and IFF we
want to merge it that way.

Should've made it clear in the log, sorry.

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] scsi: sas: scsi_queue_work can fail, so make callers aware

[Martin K. Petersen]

Johannes,

> libsas uses scsi_queue_work() to queue it's internal event
> notifications. scsi_queue_work() can return -EINVAL if the work queue
> doesn't exist and it does call queue_work() which can return false if
> the work is already queued.

Applied to 4.13/scsi-queue, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering