subject:"\[PATCH v2 1\/2\] libsas\: Don't process sas events in static works"

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

2017-06-15 Thread wangyijing



在 2017/6/15 16:00, John Garry 写道:
> On 15/06/2017 08:37, wangyijing wrote:
>>
>>
>> 在 2017/6/14 21:08, John Garry 写道:
>>> On 14/06/2017 10:04, wangyijing wrote:
>>  static void notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event 
>> event)
  {
 +struct sas_ha_event *ev;
 +
  BUG_ON(event >= HA_NUM_EVENTS);

 -sas_queue_event(event, _ha->pending,
 -_ha->ha_events[event].work, sas_ha);
 +ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
 +if (!ev)
 +return;
>> GFP_ATOMIC allocations can fail and then no events will be queued *and* 
>> we
>> don't report the error back to the caller.
>>
 Yes, it's really a problem, but I don't find a better solution, do you 
 have some suggestion ?

>>>
>>> Dan raised an issue with this approach, regarding a malfunctioning PHY 
>>> which spews out events. I still don't think we're handling it safely. 
>>> Here's the suggestion:
>>> - each asd_sas_phy owns a finite-sized pool of events
>>> - when the event pool becomes exhausted, libsas stops queuing events 
>>> (obviously) and disables the PHY in the LLDD
>>> - upon attempting to re-enable the PHY from sysfs, libsas first checks that 
>>> the pool is still not exhausted
>>>
>>> If you cannot find a good solution, then let us know and we can help.
>>
>> Hi John and Dan, what's event you found on malfunctioning PHY, if the event 
>> is PORTE_BROADCAST_RCVD, since
>> every PORTE_BROADCAST_RCVD libsas always call sas_revalidate_domain(), what 
>> about keeping a broadcast waiting(not queued in workqueue)
>> and discard others. If the event is other types, things may become knotty.
>>
> 
> As I mentioned in the v1 series discussion, I found a poorly connected 
> expander PHY was spewing out PHY up and loss of signal events continuously. 
> This is the sort of situation we should protect against. Current solution is 
> ok, as it uses a static event per port/PHY/HA.
> 
> The point is that we cannot allow a PHY to continuously send events to 
> libsas, which may lead to memory exhaustion.

The current solution won't introduce memory exhaustion, but it's not ok, since 
the root of this issue is it may lost event which is normal.
If we cannot identify the abnormal PHY, I think your mem pool idea is a 
candidate solution.

> 
> John
> 
>>
>>>
>>> John
>>>
>>>
>>> .
>>>
>>
>>
>> .
>>
> 
> 
> 
> .
>

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

2017-06-15 Thread wangyijing



在 2017/6/15 16:00, John Garry 写道:
> On 15/06/2017 08:37, wangyijing wrote:
>>
>>
>> 在 2017/6/14 21:08, John Garry 写道:
>>> On 14/06/2017 10:04, wangyijing wrote:
>>  static void notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event 
>> event)
  {
 +struct sas_ha_event *ev;
 +
  BUG_ON(event >= HA_NUM_EVENTS);

 -sas_queue_event(event, _ha->pending,
 -_ha->ha_events[event].work, sas_ha);
 +ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
 +if (!ev)
 +return;
>> GFP_ATOMIC allocations can fail and then no events will be queued *and* 
>> we
>> don't report the error back to the caller.
>>
 Yes, it's really a problem, but I don't find a better solution, do you 
 have some suggestion ?

>>>
>>> Dan raised an issue with this approach, regarding a malfunctioning PHY 
>>> which spews out events. I still don't think we're handling it safely. 
>>> Here's the suggestion:
>>> - each asd_sas_phy owns a finite-sized pool of events
>>> - when the event pool becomes exhausted, libsas stops queuing events 
>>> (obviously) and disables the PHY in the LLDD
>>> - upon attempting to re-enable the PHY from sysfs, libsas first checks that 
>>> the pool is still not exhausted
>>>
>>> If you cannot find a good solution, then let us know and we can help.
>>
>> Hi John and Dan, what's event you found on malfunctioning PHY, if the event 
>> is PORTE_BROADCAST_RCVD, since
>> every PORTE_BROADCAST_RCVD libsas always call sas_revalidate_domain(), what 
>> about keeping a broadcast waiting(not queued in workqueue)
>> and discard others. If the event is other types, things may become knotty.
>>
> 
> As I mentioned in the v1 series discussion, I found a poorly connected 
> expander PHY was spewing out PHY up and loss of signal events continuously. 
> This is the sort of situation we should protect against. Current solution is 
> ok, as it uses a static event per port/PHY/HA.
> 
> The point is that we cannot allow a PHY to continuously send events to 
> libsas, which may lead to memory exhaustion.

The current solution won't introduce memory exhaustion, but it's not ok, since 
the root of this issue is it may lost event which is normal.
If we cannot identify the abnormal PHY, I think your mem pool idea is a 
candidate solution.

> 
> John
> 
>>
>>>
>>> John
>>>
>>>
>>> .
>>>
>>
>>
>> .
>>
> 
> 
> 
> .
>

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

2017-06-15 Thread John Garry


On 15/06/2017 08:37, wangyijing wrote:



在 2017/6/14 21:08, John Garry 写道:

On 14/06/2017 10:04, wangyijing wrote:

 static void notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event event)

 {
+struct sas_ha_event *ev;
+
 BUG_ON(event >= HA_NUM_EVENTS);

-sas_queue_event(event, _ha->pending,
-_ha->ha_events[event].work, sas_ha);
+ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
+if (!ev)
+return;

GFP_ATOMIC allocations can fail and then no events will be queued *and* we
don't report the error back to the caller.


Yes, it's really a problem, but I don't find a better solution, do you have 
some suggestion ?



Dan raised an issue with this approach, regarding a malfunctioning PHY which 
spews out events. I still don't think we're handling it safely. Here's the 
suggestion:
- each asd_sas_phy owns a finite-sized pool of events
- when the event pool becomes exhausted, libsas stops queuing events 
(obviously) and disables the PHY in the LLDD
- upon attempting to re-enable the PHY from sysfs, libsas first checks that the 
pool is still not exhausted

If you cannot find a good solution, then let us know and we can help.


Hi John and Dan, what's event you found on malfunctioning PHY, if the event is 
PORTE_BROADCAST_RCVD, since
every PORTE_BROADCAST_RCVD libsas always call sas_revalidate_domain(), what 
about keeping a broadcast waiting(not queued in workqueue)
and discard others. If the event is other types, things may become knotty.



As I mentioned in the v1 series discussion, I found a poorly connected 
expander PHY was spewing out PHY up and loss of signal events 
continuously. This is the sort of situation we should protect against. 
Current solution is ok, as it uses a static event per port/PHY/HA.


The point is that we cannot allow a PHY to continuously send events to 
libsas, which may lead to memory exhaustion.


John





John


.




.

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

2017-06-15 Thread John Garry


On 15/06/2017 08:37, wangyijing wrote:



在 2017/6/14 21:08, John Garry 写道:

On 14/06/2017 10:04, wangyijing wrote:

 static void notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event event)

 {
+struct sas_ha_event *ev;
+
 BUG_ON(event >= HA_NUM_EVENTS);

-sas_queue_event(event, _ha->pending,
-_ha->ha_events[event].work, sas_ha);
+ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
+if (!ev)
+return;

GFP_ATOMIC allocations can fail and then no events will be queued *and* we
don't report the error back to the caller.


Yes, it's really a problem, but I don't find a better solution, do you have 
some suggestion ?



Dan raised an issue with this approach, regarding a malfunctioning PHY which 
spews out events. I still don't think we're handling it safely. Here's the 
suggestion:
- each asd_sas_phy owns a finite-sized pool of events
- when the event pool becomes exhausted, libsas stops queuing events 
(obviously) and disables the PHY in the LLDD
- upon attempting to re-enable the PHY from sysfs, libsas first checks that the 
pool is still not exhausted

If you cannot find a good solution, then let us know and we can help.


Hi John and Dan, what's event you found on malfunctioning PHY, if the event is 
PORTE_BROADCAST_RCVD, since
every PORTE_BROADCAST_RCVD libsas always call sas_revalidate_domain(), what 
about keeping a broadcast waiting(not queued in workqueue)
and discard others. If the event is other types, things may become knotty.



As I mentioned in the v1 series discussion, I found a poorly connected 
expander PHY was spewing out PHY up and loss of signal events 
continuously. This is the sort of situation we should protect against. 
Current solution is ok, as it uses a static event per port/PHY/HA.


The point is that we cannot allow a PHY to continuously send events to 
libsas, which may lead to memory exhaustion.


John





John


.




.

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

2017-06-15 Thread wangyijing



在 2017/6/14 21:08, John Garry 写道:
> On 14/06/2017 10:04, wangyijing wrote:
  static void notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event 
 event)
 >>  {
 >> +struct sas_ha_event *ev;
 >> +
 >>  BUG_ON(event >= HA_NUM_EVENTS);
 >>
 >> -sas_queue_event(event, _ha->pending,
 >> -_ha->ha_events[event].work, sas_ha);
 >> +ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
 >> +if (!ev)
 >> +return;
>>> > GFP_ATOMIC allocations can fail and then no events will be queued *and* we
>>> > don't report the error back to the caller.
>>> >
>> Yes, it's really a problem, but I don't find a better solution, do you have 
>> some suggestion ?
>>
> 
> Dan raised an issue with this approach, regarding a malfunctioning PHY which 
> spews out events. I still don't think we're handling it safely. Here's the 
> suggestion:
> - each asd_sas_phy owns a finite-sized pool of events
> - when the event pool becomes exhausted, libsas stops queuing events 
> (obviously) and disables the PHY in the LLDD
> - upon attempting to re-enable the PHY from sysfs, libsas first checks that 
> the pool is still not exhausted
> 
> If you cannot find a good solution, then let us know and we can help.

Hi John and Dan, what's event you found on malfunctioning PHY, if the event is 
PORTE_BROADCAST_RCVD, since
every PORTE_BROADCAST_RCVD libsas always call sas_revalidate_domain(), what 
about keeping a broadcast waiting(not queued in workqueue)
and discard others. If the event is other types, things may become knotty.


> 
> John
> 
> 
> .
>

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

2017-06-15 Thread wangyijing



在 2017/6/14 21:08, John Garry 写道:
> On 14/06/2017 10:04, wangyijing wrote:
  static void notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event 
 event)
 >>  {
 >> +struct sas_ha_event *ev;
 >> +
 >>  BUG_ON(event >= HA_NUM_EVENTS);
 >>
 >> -sas_queue_event(event, _ha->pending,
 >> -_ha->ha_events[event].work, sas_ha);
 >> +ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
 >> +if (!ev)
 >> +return;
>>> > GFP_ATOMIC allocations can fail and then no events will be queued *and* we
>>> > don't report the error back to the caller.
>>> >
>> Yes, it's really a problem, but I don't find a better solution, do you have 
>> some suggestion ?
>>
> 
> Dan raised an issue with this approach, regarding a malfunctioning PHY which 
> spews out events. I still don't think we're handling it safely. Here's the 
> suggestion:
> - each asd_sas_phy owns a finite-sized pool of events
> - when the event pool becomes exhausted, libsas stops queuing events 
> (obviously) and disables the PHY in the LLDD
> - upon attempting to re-enable the PHY from sysfs, libsas first checks that 
> the pool is still not exhausted
> 
> If you cannot find a good solution, then let us know and we can help.

Hi John and Dan, what's event you found on malfunctioning PHY, if the event is 
PORTE_BROADCAST_RCVD, since
every PORTE_BROADCAST_RCVD libsas always call sas_revalidate_domain(), what 
about keeping a broadcast waiting(not queued in workqueue)
and discard others. If the event is other types, things may become knotty.


> 
> John
> 
> 
> .
>

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

2017-06-14 Thread John Garry


On 14/06/2017 10:04, wangyijing wrote:

 static void notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event event)
>>  {
>> +  struct sas_ha_event *ev;
>> +
>>BUG_ON(event >= HA_NUM_EVENTS);
>>
>> -  sas_queue_event(event, _ha->pending,
>> -  _ha->ha_events[event].work, sas_ha);
>> +  ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
>> +  if (!ev)
>> +  return;

> GFP_ATOMIC allocations can fail and then no events will be queued *and* we
> don't report the error back to the caller.
>

Yes, it's really a problem, but I don't find a better solution, do you have 
some suggestion ?



Dan raised an issue with this approach, regarding a malfunctioning PHY 
which spews out events. I still don't think we're handling it safely. 
Here's the suggestion:

- each asd_sas_phy owns a finite-sized pool of events
- when the event pool becomes exhausted, libsas stops queuing events 
(obviously) and disables the PHY in the LLDD
- upon attempting to re-enable the PHY from sysfs, libsas first checks 
that the pool is still not exhausted


If you cannot find a good solution, then let us know and we can help.

John

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

2017-06-14 Thread John Garry


On 14/06/2017 10:04, wangyijing wrote:

 static void notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event event)
>>  {
>> +  struct sas_ha_event *ev;
>> +
>>BUG_ON(event >= HA_NUM_EVENTS);
>>
>> -  sas_queue_event(event, _ha->pending,
>> -  _ha->ha_events[event].work, sas_ha);
>> +  ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
>> +  if (!ev)
>> +  return;

> GFP_ATOMIC allocations can fail and then no events will be queued *and* we
> don't report the error back to the caller.
>

Yes, it's really a problem, but I don't find a better solution, do you have 
some suggestion ?



Dan raised an issue with this approach, regarding a malfunctioning PHY 
which spews out events. I still don't think we're handling it safely. 
Here's the suggestion:

- each asd_sas_phy owns a finite-sized pool of events
- when the event pool becomes exhausted, libsas stops queuing events 
(obviously) and disables the PHY in the LLDD
- upon attempting to re-enable the PHY from sysfs, libsas first checks 
that the pool is still not exhausted


If you cannot find a good solution, then let us know and we can help.

John

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

2017-06-14 Thread wangyijing



在 2017/6/14 17:18, Johannes Thumshirn 写道:
> On 06/14/2017 11:04 AM, wangyijing wrote:
  static void notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event 
 event)
  {
 +  struct sas_ha_event *ev;
 +
BUG_ON(event >= HA_NUM_EVENTS);
  
 -  sas_queue_event(event, _ha->pending,
 -  _ha->ha_events[event].work, sas_ha);
 +  ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
 +  if (!ev)
 +  return;
>>> GFP_ATOMIC allocations can fail and then no events will be queued *and* we
>>> don't report the error back to the caller.
>>>
>>
>> Yes, it's really a problem, but I don't find a better solution, do you have 
>> some suggestion ?
> 
> Something like this?
> 
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c
> b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index d622db502ec9..27cd7307d308 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> @@ -894,7 +894,11 @@ static int hisi_sas_controller_reset(struct
> hisi_hba *hisi_hba)
> hisi_sas_release_tasks(hisi_hba);
> spin_unlock_irqrestore(_hba->lock, flags);
> 
> -   sas_ha->notify_ha_event(sas_ha, HAE_RESET);
> +   rc = sas_ha->notify_ha_event(sas_ha, HAE_RESET);
> +   if (rc) {
> +   dev_warn(dev, "failed to queue HA event (%d)\n",
> rc);
> +   goto out;
> +   }
> dev_dbg(dev, "controller reset successful!\n");
> } else
> return -1;
> diff --git a/drivers/scsi/libsas/sas_event.c
> b/drivers/scsi/libsas/sas_event.c
> index aadbd5314c5c..6c91fb31f891 100644
> --- a/drivers/scsi/libsas/sas_event.c
> +++ b/drivers/scsi/libsas/sas_event.c
> @@ -116,7 +116,7 @@ void sas_enable_revalidation(struct sas_ha_struct *ha)
> mutex_unlock(>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);
> 
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index dd0f72c95abe..5d9cd6d20855 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -415,7 +415,7 @@ 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);
> +   int (*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);

This make sense, return a fail status and report it to the callers.


> 
>

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

2017-06-14 Thread wangyijing



在 2017/6/14 17:18, Johannes Thumshirn 写道:
> On 06/14/2017 11:04 AM, wangyijing wrote:
  static void notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event 
 event)
  {
 +  struct sas_ha_event *ev;
 +
BUG_ON(event >= HA_NUM_EVENTS);
  
 -  sas_queue_event(event, _ha->pending,
 -  _ha->ha_events[event].work, sas_ha);
 +  ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
 +  if (!ev)
 +  return;
>>> GFP_ATOMIC allocations can fail and then no events will be queued *and* we
>>> don't report the error back to the caller.
>>>
>>
>> Yes, it's really a problem, but I don't find a better solution, do you have 
>> some suggestion ?
> 
> Something like this?
> 
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c
> b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index d622db502ec9..27cd7307d308 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> @@ -894,7 +894,11 @@ static int hisi_sas_controller_reset(struct
> hisi_hba *hisi_hba)
> hisi_sas_release_tasks(hisi_hba);
> spin_unlock_irqrestore(_hba->lock, flags);
> 
> -   sas_ha->notify_ha_event(sas_ha, HAE_RESET);
> +   rc = sas_ha->notify_ha_event(sas_ha, HAE_RESET);
> +   if (rc) {
> +   dev_warn(dev, "failed to queue HA event (%d)\n",
> rc);
> +   goto out;
> +   }
> dev_dbg(dev, "controller reset successful!\n");
> } else
> return -1;
> diff --git a/drivers/scsi/libsas/sas_event.c
> b/drivers/scsi/libsas/sas_event.c
> index aadbd5314c5c..6c91fb31f891 100644
> --- a/drivers/scsi/libsas/sas_event.c
> +++ b/drivers/scsi/libsas/sas_event.c
> @@ -116,7 +116,7 @@ void sas_enable_revalidation(struct sas_ha_struct *ha)
> mutex_unlock(>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);
> 
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index dd0f72c95abe..5d9cd6d20855 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -415,7 +415,7 @@ 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);
> +   int (*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);

This make sense, return a fail status and report it to the callers.


> 
>

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

2017-06-14 Thread Johannes Thumshirn

On 06/14/2017 11:04 AM, wangyijing wrote:
>>>  static void notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event 
>>> event)
>>>  {
>>> +   struct sas_ha_event *ev;
>>> +
>>> BUG_ON(event >= HA_NUM_EVENTS);
>>>  
>>> -   sas_queue_event(event, _ha->pending,
>>> -   _ha->ha_events[event].work, sas_ha);
>>> +   ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
>>> +   if (!ev)
>>> +   return;
>> GFP_ATOMIC allocations can fail and then no events will be queued *and* we
>> don't report the error back to the caller.
>>
> 
> Yes, it's really a problem, but I don't find a better solution, do you have 
> some suggestion ?

Something like this?

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c
b/drivers/scsi/hisi_sas/hisi_sas_main.c
index d622db502ec9..27cd7307d308 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -894,7 +894,11 @@ static int hisi_sas_controller_reset(struct
hisi_hba *hisi_hba)
hisi_sas_release_tasks(hisi_hba);
spin_unlock_irqrestore(_hba->lock, flags);

-   sas_ha->notify_ha_event(sas_ha, HAE_RESET);
+   rc = sas_ha->notify_ha_event(sas_ha, HAE_RESET);
+   if (rc) {
+   dev_warn(dev, "failed to queue HA event (%d)\n",
rc);
+   goto out;
+   }
dev_dbg(dev, "controller reset successful!\n");
} else
return -1;
diff --git a/drivers/scsi/libsas/sas_event.c
b/drivers/scsi/libsas/sas_event.c
index aadbd5314c5c..6c91fb31f891 100644
--- a/drivers/scsi/libsas/sas_event.c
+++ b/drivers/scsi/libsas/sas_event.c
@@ -116,7 +116,7 @@ void sas_enable_revalidation(struct sas_ha_struct *ha)
mutex_unlock(>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);

diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index dd0f72c95abe..5d9cd6d20855 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -415,7 +415,7 @@ 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);
+   int (*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);


-- 
Johannes Thumshirn  Storage
jthumsh...@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 v2 1/2] libsas: Don't process sas events in static works

2017-06-14 Thread Johannes Thumshirn

On 06/14/2017 11:04 AM, wangyijing wrote:
>>>  static void notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event 
>>> event)
>>>  {
>>> +   struct sas_ha_event *ev;
>>> +
>>> BUG_ON(event >= HA_NUM_EVENTS);
>>>  
>>> -   sas_queue_event(event, _ha->pending,
>>> -   _ha->ha_events[event].work, sas_ha);
>>> +   ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
>>> +   if (!ev)
>>> +   return;
>> GFP_ATOMIC allocations can fail and then no events will be queued *and* we
>> don't report the error back to the caller.
>>
> 
> Yes, it's really a problem, but I don't find a better solution, do you have 
> some suggestion ?

Something like this?

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c
b/drivers/scsi/hisi_sas/hisi_sas_main.c
index d622db502ec9..27cd7307d308 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -894,7 +894,11 @@ static int hisi_sas_controller_reset(struct
hisi_hba *hisi_hba)
hisi_sas_release_tasks(hisi_hba);
spin_unlock_irqrestore(_hba->lock, flags);

-   sas_ha->notify_ha_event(sas_ha, HAE_RESET);
+   rc = sas_ha->notify_ha_event(sas_ha, HAE_RESET);
+   if (rc) {
+   dev_warn(dev, "failed to queue HA event (%d)\n",
rc);
+   goto out;
+   }
dev_dbg(dev, "controller reset successful!\n");
} else
return -1;
diff --git a/drivers/scsi/libsas/sas_event.c
b/drivers/scsi/libsas/sas_event.c
index aadbd5314c5c..6c91fb31f891 100644
--- a/drivers/scsi/libsas/sas_event.c
+++ b/drivers/scsi/libsas/sas_event.c
@@ -116,7 +116,7 @@ void sas_enable_revalidation(struct sas_ha_struct *ha)
mutex_unlock(>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);

diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index dd0f72c95abe..5d9cd6d20855 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -415,7 +415,7 @@ 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);
+   int (*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);


-- 
Johannes Thumshirn  Storage
jthumsh...@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 v2 1/2] libsas: Don't process sas events in static works

2017-06-14 Thread wangyijing

>>  static void notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event 
>> event)
>>  {
>> +struct sas_ha_event *ev;
>> +
>>  BUG_ON(event >= HA_NUM_EVENTS);
>>  
>> -sas_queue_event(event, _ha->pending,
>> -_ha->ha_events[event].work, sas_ha);
>> +ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
>> +if (!ev)
>> +return;
> GFP_ATOMIC allocations can fail and then no events will be queued *and* we
> don't report the error back to the caller.
> 

Yes, it's really a problem, but I don't find a better solution, do you have 
some suggestion ?

> 
> 
>> index 64e9cdd..c227a8b 100644
>> --- a/drivers/scsi/libsas/sas_init.c
>> +++ b/drivers/scsi/libsas/sas_init.c
>> @@ -111,10 +111,6 @@ void sas_hash_addr(u8 *hashed, const u8 *sas_addr)
>>  
>>  void sas_hae_reset(struct work_struct *work)
>>  {
>> -struct sas_ha_event *ev = to_sas_ha_event(work);
>> -struct sas_ha_struct *ha = ev->ha;
>> -
>> -clear_bit(HAE_RESET, >pending);
>>  }
> 
> I don't really get why you need a stubbed out sas_hae_reset(). Can't we
> just kill it if it doesn't have anything left to do?

I have no idea about this function history, I agree clean it out.

> 
>

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

2017-06-14 Thread wangyijing

>>  static void notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event 
>> event)
>>  {
>> +struct sas_ha_event *ev;
>> +
>>  BUG_ON(event >= HA_NUM_EVENTS);
>>  
>> -sas_queue_event(event, _ha->pending,
>> -_ha->ha_events[event].work, sas_ha);
>> +ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
>> +if (!ev)
>> +return;
> GFP_ATOMIC allocations can fail and then no events will be queued *and* we
> don't report the error back to the caller.
> 

Yes, it's really a problem, but I don't find a better solution, do you have 
some suggestion ?

> 
> 
>> index 64e9cdd..c227a8b 100644
>> --- a/drivers/scsi/libsas/sas_init.c
>> +++ b/drivers/scsi/libsas/sas_init.c
>> @@ -111,10 +111,6 @@ void sas_hash_addr(u8 *hashed, const u8 *sas_addr)
>>  
>>  void sas_hae_reset(struct work_struct *work)
>>  {
>> -struct sas_ha_event *ev = to_sas_ha_event(work);
>> -struct sas_ha_struct *ha = ev->ha;
>> -
>> -clear_bit(HAE_RESET, >pending);
>>  }
> 
> I don't really get why you need a stubbed out sas_hae_reset(). Can't we
> just kill it if it doesn't have anything left to do?

I have no idea about this function history, I agree clean it out.

> 
>

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

2017-06-14 Thread Johannes Thumshirn

On 06/14/2017 09:33 AM, Yijing Wang wrote:
> Now libsas hotplug work is static, LLDD driver queue
> the hotplug work into shost->work_q. If LLDD driver
> burst post lots hotplug events to libsas, the hotplug
> events may pending in the workqueue like
> 
> shost->work_q
> new work[PORTE_BYTES_DMAED] --> |[PHYE_LOSS_OF_SIGNAL][PORTE_BYTES_DMAED] -> 
> processing
> |<---wait worker to process>|
> In this case, a new PORTE_BYTES_DMAED event coming, libsas try to queue it
> to shost->work_q, but this work is already pending, so it would be lost.
> Finally, libsas delete the related sas port and sas devices, but LLDD driver
> expect libsas add the sas port and devices(last sas event).
> 
> This patch remove the static defined hotplug work, and use dynamic work to
> avoid missing hotplug events.
> 
> Signed-off-by: Yijing Wang 
> Signed-off-by: Yousong He 
> Signed-off-by: Qilin Chen 
> ---

[...]

>  static void notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event 
> event)
>  {
> + struct sas_ha_event *ev;
> +
>   BUG_ON(event >= HA_NUM_EVENTS);
>  
> - sas_queue_event(event, _ha->pending,
> - _ha->ha_events[event].work, sas_ha);
> + ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
> + if (!ev)
> + return;
GFP_ATOMIC allocations can fail and then no events will be queued *and* we
don't report the error back to the caller.



> index 64e9cdd..c227a8b 100644
> --- a/drivers/scsi/libsas/sas_init.c
> +++ b/drivers/scsi/libsas/sas_init.c
> @@ -111,10 +111,6 @@ void sas_hash_addr(u8 *hashed, const u8 *sas_addr)
>  
>  void sas_hae_reset(struct work_struct *work)
>  {
> - struct sas_ha_event *ev = to_sas_ha_event(work);
> - struct sas_ha_struct *ha = ev->ha;
> -
> - clear_bit(HAE_RESET, >pending);
>  }

I don't really get why you need a stubbed out sas_hae_reset(). Can't we
just kill it if it doesn't have anything left to do?


-- 
Johannes Thumshirn  Storage
jthumsh...@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 v2 1/2] libsas: Don't process sas events in static works

2017-06-14 Thread Johannes Thumshirn

On 06/14/2017 09:33 AM, Yijing Wang wrote:
> Now libsas hotplug work is static, LLDD driver queue
> the hotplug work into shost->work_q. If LLDD driver
> burst post lots hotplug events to libsas, the hotplug
> events may pending in the workqueue like
> 
> shost->work_q
> new work[PORTE_BYTES_DMAED] --> |[PHYE_LOSS_OF_SIGNAL][PORTE_BYTES_DMAED] -> 
> processing
> |<---wait worker to process>|
> In this case, a new PORTE_BYTES_DMAED event coming, libsas try to queue it
> to shost->work_q, but this work is already pending, so it would be lost.
> Finally, libsas delete the related sas port and sas devices, but LLDD driver
> expect libsas add the sas port and devices(last sas event).
> 
> This patch remove the static defined hotplug work, and use dynamic work to
> avoid missing hotplug events.
> 
> Signed-off-by: Yijing Wang 
> Signed-off-by: Yousong He 
> Signed-off-by: Qilin Chen 
> ---

[...]

>  static void notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event 
> event)
>  {
> + struct sas_ha_event *ev;
> +
>   BUG_ON(event >= HA_NUM_EVENTS);
>  
> - sas_queue_event(event, _ha->pending,
> - _ha->ha_events[event].work, sas_ha);
> + ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
> + if (!ev)
> + return;
GFP_ATOMIC allocations can fail and then no events will be queued *and* we
don't report the error back to the caller.



> index 64e9cdd..c227a8b 100644
> --- a/drivers/scsi/libsas/sas_init.c
> +++ b/drivers/scsi/libsas/sas_init.c
> @@ -111,10 +111,6 @@ void sas_hash_addr(u8 *hashed, const u8 *sas_addr)
>  
>  void sas_hae_reset(struct work_struct *work)
>  {
> - struct sas_ha_event *ev = to_sas_ha_event(work);
> - struct sas_ha_struct *ha = ev->ha;
> -
> - clear_bit(HAE_RESET, >pending);
>  }

I don't really get why you need a stubbed out sas_hae_reset(). Can't we
just kill it if it doesn't have anything left to do?


-- 
Johannes Thumshirn  Storage
jthumsh...@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

[Resend][PATCH v2 1/2] libsas: Don't process sas events in static works

2017-06-14 Thread Yijing Wang

Now libsas hotplug work is static, LLDD driver queue
the hotplug work into shost->work_q. If LLDD driver
burst post lots hotplug events to libsas, the hotplug
events may pending in the workqueue like

shost->work_q
new work[PORTE_BYTES_DMAED] --> |[PHYE_LOSS_OF_SIGNAL][PORTE_BYTES_DMAED] -> 
processing
|<---wait worker to process>|
In this case, a new PORTE_BYTES_DMAED event coming, libsas try to queue it
to shost->work_q, but this work is already pending, so it would be lost.
Finally, libsas delete the related sas port and sas devices, but LLDD driver
expect libsas add the sas port and devices(last sas event).

This patch remove the static defined hotplug work, and use dynamic work to
avoid missing hotplug events.

Signed-off-by: Yijing Wang 
Signed-off-by: Yousong He 
Signed-off-by: Qilin Chen 
---
 drivers/scsi/libsas/sas_event.c| 88 +++---
 drivers/scsi/libsas/sas_init.c |  6 ---
 drivers/scsi/libsas/sas_internal.h |  3 ++
 drivers/scsi/libsas/sas_phy.c  | 45 ---
 drivers/scsi/libsas/sas_port.c | 18 
 include/scsi/libsas.h  | 10 +
 6 files changed, 84 insertions(+), 86 deletions(-)

diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
index aadbd53..06c5c4b 100644
--- a/drivers/scsi/libsas/sas_event.c
+++ b/drivers/scsi/libsas/sas_event.c
@@ -27,6 +27,10 @@
 #include "sas_internal.h"
 #include "sas_dump.h"
 
+static const work_func_t sas_ha_event_fns[HA_NUM_EVENTS] = {
+   [HAE_RESET] = sas_hae_reset,
+};
+
 void sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw)
 {
if (!test_bit(SAS_HA_REGISTERED, >state))
@@ -40,17 +44,14 @@ void sas_queue_work(struct sas_ha_struct *ha, struct 
sas_work *sw)
scsi_queue_work(ha->core.shost, >work);
 }
 
-static void sas_queue_event(int event, unsigned long *pending,
-   struct sas_work *work,
+static void sas_queue_event(int event, struct sas_work *work,
struct sas_ha_struct *ha)
 {
-   if (!test_and_set_bit(event, pending)) {
-   unsigned long flags;
+   unsigned long flags;
 
-   spin_lock_irqsave(>lock, flags);
-   sas_queue_work(ha, work);
-   spin_unlock_irqrestore(>lock, flags);
-   }
+   spin_lock_irqsave(>lock, flags);
+   sas_queue_work(ha, work);
+   spin_unlock_irqrestore(>lock, flags);
 }
 
 
@@ -111,52 +112,87 @@ void sas_enable_revalidation(struct sas_ha_struct *ha)
if (!test_and_clear_bit(ev, >pending))
continue;
 
-   sas_queue_event(ev, >pending, >disc_work[ev].work, ha);
+   sas_queue_event(ev, >disc_work[ev].work, ha);
}
mutex_unlock(>disco_mutex);
 }
 
+static void sas_ha_event_worker(struct work_struct *work)
+{
+   struct sas_ha_event *ev = to_sas_ha_event(work);
+
+   sas_ha_event_fns[ev->type](work);
+   kfree(ev);
+}
+
+static void sas_port_event_worker(struct work_struct *work)
+{
+   struct asd_sas_event *ev = to_asd_sas_event(work);
+
+   sas_port_event_fns[ev->type](work);
+   kfree(ev);
+}
+
+static void sas_phy_event_worker(struct work_struct *work)
+{
+   struct asd_sas_event *ev = to_asd_sas_event(work);
+
+   sas_phy_event_fns[ev->type](work);
+   kfree(ev);
+}
+
 static void notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event event)
 {
+   struct sas_ha_event *ev;
+
BUG_ON(event >= HA_NUM_EVENTS);
 
-   sas_queue_event(event, _ha->pending,
-   _ha->ha_events[event].work, sas_ha);
+   ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
+   if (!ev)
+   return;
+
+   INIT_SAS_WORK(>work, sas_ha_event_worker);
+   ev->ha = sas_ha;
+   ev->type = event;
+   sas_queue_event(event, >work, sas_ha);
 }
 
 static void notify_port_event(struct asd_sas_phy *phy, enum port_event event)
 {
+   struct asd_sas_event *ev;
struct sas_ha_struct *ha = phy->ha;
 
BUG_ON(event >= PORT_NUM_EVENTS);
 
-   sas_queue_event(event, >port_events_pending,
-   >port_events[event].work, ha);
+   ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
+   if (!ev)
+   return;
+
+   INIT_SAS_WORK(>work, sas_port_event_worker);
+   ev->phy = phy;
+   ev->type = event;
+   sas_queue_event(event, >work, ha);
 }
 
 void sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event)
 {
+   struct asd_sas_event *ev;
struct sas_ha_struct *ha = phy->ha;
 
BUG_ON(event >= PHY_NUM_EVENTS);
 
-   sas_queue_event(event, >phy_events_pending,
-   >phy_events[event].work, ha);
+   ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
+   if (!ev)
+   return;
+
+   INIT_SAS_WORK(>work,

[Resend][PATCH v2 1/2] libsas: Don't process sas events in static works

2017-06-14 Thread Yijing Wang

Now libsas hotplug work is static, LLDD driver queue
the hotplug work into shost->work_q. If LLDD driver
burst post lots hotplug events to libsas, the hotplug
events may pending in the workqueue like

shost->work_q
new work[PORTE_BYTES_DMAED] --> |[PHYE_LOSS_OF_SIGNAL][PORTE_BYTES_DMAED] -> 
processing
|<---wait worker to process>|
In this case, a new PORTE_BYTES_DMAED event coming, libsas try to queue it
to shost->work_q, but this work is already pending, so it would be lost.
Finally, libsas delete the related sas port and sas devices, but LLDD driver
expect libsas add the sas port and devices(last sas event).

This patch remove the static defined hotplug work, and use dynamic work to
avoid missing hotplug events.

Signed-off-by: Yijing Wang 
Signed-off-by: Yousong He 
Signed-off-by: Qilin Chen 
---
 drivers/scsi/libsas/sas_event.c| 88 +++---
 drivers/scsi/libsas/sas_init.c |  6 ---
 drivers/scsi/libsas/sas_internal.h |  3 ++
 drivers/scsi/libsas/sas_phy.c  | 45 ---
 drivers/scsi/libsas/sas_port.c | 18 
 include/scsi/libsas.h  | 10 +
 6 files changed, 84 insertions(+), 86 deletions(-)

diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
index aadbd53..06c5c4b 100644
--- a/drivers/scsi/libsas/sas_event.c
+++ b/drivers/scsi/libsas/sas_event.c
@@ -27,6 +27,10 @@
 #include "sas_internal.h"
 #include "sas_dump.h"
 
+static const work_func_t sas_ha_event_fns[HA_NUM_EVENTS] = {
+   [HAE_RESET] = sas_hae_reset,
+};
+
 void sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw)
 {
if (!test_bit(SAS_HA_REGISTERED, >state))
@@ -40,17 +44,14 @@ void sas_queue_work(struct sas_ha_struct *ha, struct 
sas_work *sw)
scsi_queue_work(ha->core.shost, >work);
 }
 
-static void sas_queue_event(int event, unsigned long *pending,
-   struct sas_work *work,
+static void sas_queue_event(int event, struct sas_work *work,
struct sas_ha_struct *ha)
 {
-   if (!test_and_set_bit(event, pending)) {
-   unsigned long flags;
+   unsigned long flags;
 
-   spin_lock_irqsave(>lock, flags);
-   sas_queue_work(ha, work);
-   spin_unlock_irqrestore(>lock, flags);
-   }
+   spin_lock_irqsave(>lock, flags);
+   sas_queue_work(ha, work);
+   spin_unlock_irqrestore(>lock, flags);
 }
 
 
@@ -111,52 +112,87 @@ void sas_enable_revalidation(struct sas_ha_struct *ha)
if (!test_and_clear_bit(ev, >pending))
continue;
 
-   sas_queue_event(ev, >pending, >disc_work[ev].work, ha);
+   sas_queue_event(ev, >disc_work[ev].work, ha);
}
mutex_unlock(>disco_mutex);
 }
 
+static void sas_ha_event_worker(struct work_struct *work)
+{
+   struct sas_ha_event *ev = to_sas_ha_event(work);
+
+   sas_ha_event_fns[ev->type](work);
+   kfree(ev);
+}
+
+static void sas_port_event_worker(struct work_struct *work)
+{
+   struct asd_sas_event *ev = to_asd_sas_event(work);
+
+   sas_port_event_fns[ev->type](work);
+   kfree(ev);
+}
+
+static void sas_phy_event_worker(struct work_struct *work)
+{
+   struct asd_sas_event *ev = to_asd_sas_event(work);
+
+   sas_phy_event_fns[ev->type](work);
+   kfree(ev);
+}
+
 static void notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event event)
 {
+   struct sas_ha_event *ev;
+
BUG_ON(event >= HA_NUM_EVENTS);
 
-   sas_queue_event(event, _ha->pending,
-   _ha->ha_events[event].work, sas_ha);
+   ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
+   if (!ev)
+   return;
+
+   INIT_SAS_WORK(>work, sas_ha_event_worker);
+   ev->ha = sas_ha;
+   ev->type = event;
+   sas_queue_event(event, >work, sas_ha);
 }
 
 static void notify_port_event(struct asd_sas_phy *phy, enum port_event event)
 {
+   struct asd_sas_event *ev;
struct sas_ha_struct *ha = phy->ha;
 
BUG_ON(event >= PORT_NUM_EVENTS);
 
-   sas_queue_event(event, >port_events_pending,
-   >port_events[event].work, ha);
+   ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
+   if (!ev)
+   return;
+
+   INIT_SAS_WORK(>work, sas_port_event_worker);
+   ev->phy = phy;
+   ev->type = event;
+   sas_queue_event(event, >work, ha);
 }
 
 void sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event)
 {
+   struct asd_sas_event *ev;
struct sas_ha_struct *ha = phy->ha;
 
BUG_ON(event >= PHY_NUM_EVENTS);
 
-   sas_queue_event(event, >phy_events_pending,
-   >phy_events[event].work, ha);
+   ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
+   if (!ev)
+   return;
+
+   INIT_SAS_WORK(>work, sas_phy_event_worker);
+   ev->phy = phy;
+   ev->type = event;
+

[PATCH v2 1/2] libsas: Don't process sas events in static works

2017-06-14 Thread Yijing Wang

Now libsas hotplug work is static, LLDD driver queue
the hotplug work into shost->work_q. If LLDD driver
burst post lots hotplug events to libsas, the hotplug
events may pending in the workqueue like

shost->work_q
new work[PORTE_BYTES_DMAED] --> |[PHYE_LOSS_OF_SIGNAL][PORTE_BYTES_DMAED] -> 
processing
|<---wait worker to process>|
In this case, a new PORTE_BYTES_DMAED event coming, libsas try to queue it
to shost->work_q, but this work is already pending, so it would be lost.
Finally, libsas delete the related sas port and sas devices, but LLDD driver
expect libsas add the sas port and devices(last sas event).

This patch remove the static defined hotplug work, and use dynamic work to
avoid missing hotplug events.

Signed-off-by: Yijing Wang 
Signed-off-by: Yousong He 
Signed-off-by: Qilin Chen 
---
 drivers/scsi/libsas/sas_event.c| 88 +++---
 drivers/scsi/libsas/sas_init.c |  6 ---
 drivers/scsi/libsas/sas_internal.h |  3 ++
 drivers/scsi/libsas/sas_phy.c  | 45 ---
 drivers/scsi/libsas/sas_port.c | 18 
 include/scsi/libsas.h  | 10 +
 6 files changed, 84 insertions(+), 86 deletions(-)

diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
index aadbd53..06c5c4b 100644
--- a/drivers/scsi/libsas/sas_event.c
+++ b/drivers/scsi/libsas/sas_event.c
@@ -27,6 +27,10 @@
 #include "sas_internal.h"
 #include "sas_dump.h"
 
+static const work_func_t sas_ha_event_fns[HA_NUM_EVENTS] = {
+   [HAE_RESET] = sas_hae_reset,
+};
+
 void sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw)
 {
if (!test_bit(SAS_HA_REGISTERED, >state))
@@ -40,17 +44,14 @@ void sas_queue_work(struct sas_ha_struct *ha, struct 
sas_work *sw)
scsi_queue_work(ha->core.shost, >work);
 }
 
-static void sas_queue_event(int event, unsigned long *pending,
-   struct sas_work *work,
+static void sas_queue_event(int event, struct sas_work *work,
struct sas_ha_struct *ha)
 {
-   if (!test_and_set_bit(event, pending)) {
-   unsigned long flags;
+   unsigned long flags;
 
-   spin_lock_irqsave(>lock, flags);
-   sas_queue_work(ha, work);
-   spin_unlock_irqrestore(>lock, flags);
-   }
+   spin_lock_irqsave(>lock, flags);
+   sas_queue_work(ha, work);
+   spin_unlock_irqrestore(>lock, flags);
 }
 
 
@@ -111,52 +112,87 @@ void sas_enable_revalidation(struct sas_ha_struct *ha)
if (!test_and_clear_bit(ev, >pending))
continue;
 
-   sas_queue_event(ev, >pending, >disc_work[ev].work, ha);
+   sas_queue_event(ev, >disc_work[ev].work, ha);
}
mutex_unlock(>disco_mutex);
 }
 
+static void sas_ha_event_worker(struct work_struct *work)
+{
+   struct sas_ha_event *ev = to_sas_ha_event(work);
+
+   sas_ha_event_fns[ev->type](work);
+   kfree(ev);
+}
+
+static void sas_port_event_worker(struct work_struct *work)
+{
+   struct asd_sas_event *ev = to_asd_sas_event(work);
+
+   sas_port_event_fns[ev->type](work);
+   kfree(ev);
+}
+
+static void sas_phy_event_worker(struct work_struct *work)
+{
+   struct asd_sas_event *ev = to_asd_sas_event(work);
+
+   sas_phy_event_fns[ev->type](work);
+   kfree(ev);
+}
+
 static void notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event event)
 {
+   struct sas_ha_event *ev;
+
BUG_ON(event >= HA_NUM_EVENTS);
 
-   sas_queue_event(event, _ha->pending,
-   _ha->ha_events[event].work, sas_ha);
+   ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
+   if (!ev)
+   return;
+
+   INIT_SAS_WORK(>work, sas_ha_event_worker);
+   ev->ha = sas_ha;
+   ev->type = event;
+   sas_queue_event(event, >work, sas_ha);
 }
 
 static void notify_port_event(struct asd_sas_phy *phy, enum port_event event)
 {
+   struct asd_sas_event *ev;
struct sas_ha_struct *ha = phy->ha;
 
BUG_ON(event >= PORT_NUM_EVENTS);
 
-   sas_queue_event(event, >port_events_pending,
-   >port_events[event].work, ha);
+   ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
+   if (!ev)
+   return;
+
+   INIT_SAS_WORK(>work, sas_port_event_worker);
+   ev->phy = phy;
+   ev->type = event;
+   sas_queue_event(event, >work, ha);
 }
 
 void sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event)
 {
+   struct asd_sas_event *ev;
struct sas_ha_struct *ha = phy->ha;
 
BUG_ON(event >= PHY_NUM_EVENTS);
 
-   sas_queue_event(event, >phy_events_pending,
-   >phy_events[event].work, ha);
+   ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
+   if (!ev)
+   return;
+
+   INIT_SAS_WORK(>work,

[PATCH v2 1/2] libsas: Don't process sas events in static works

2017-06-14 Thread Yijing Wang

Now libsas hotplug work is static, LLDD driver queue
the hotplug work into shost->work_q. If LLDD driver
burst post lots hotplug events to libsas, the hotplug
events may pending in the workqueue like

shost->work_q
new work[PORTE_BYTES_DMAED] --> |[PHYE_LOSS_OF_SIGNAL][PORTE_BYTES_DMAED] -> 
processing
|<---wait worker to process>|
In this case, a new PORTE_BYTES_DMAED event coming, libsas try to queue it
to shost->work_q, but this work is already pending, so it would be lost.
Finally, libsas delete the related sas port and sas devices, but LLDD driver
expect libsas add the sas port and devices(last sas event).

This patch remove the static defined hotplug work, and use dynamic work to
avoid missing hotplug events.

Signed-off-by: Yijing Wang 
Signed-off-by: Yousong He 
Signed-off-by: Qilin Chen 
---
 drivers/scsi/libsas/sas_event.c| 88 +++---
 drivers/scsi/libsas/sas_init.c |  6 ---
 drivers/scsi/libsas/sas_internal.h |  3 ++
 drivers/scsi/libsas/sas_phy.c  | 45 ---
 drivers/scsi/libsas/sas_port.c | 18 
 include/scsi/libsas.h  | 10 +
 6 files changed, 84 insertions(+), 86 deletions(-)

diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
index aadbd53..06c5c4b 100644
--- a/drivers/scsi/libsas/sas_event.c
+++ b/drivers/scsi/libsas/sas_event.c
@@ -27,6 +27,10 @@
 #include "sas_internal.h"
 #include "sas_dump.h"
 
+static const work_func_t sas_ha_event_fns[HA_NUM_EVENTS] = {
+   [HAE_RESET] = sas_hae_reset,
+};
+
 void sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw)
 {
if (!test_bit(SAS_HA_REGISTERED, >state))
@@ -40,17 +44,14 @@ void sas_queue_work(struct sas_ha_struct *ha, struct 
sas_work *sw)
scsi_queue_work(ha->core.shost, >work);
 }
 
-static void sas_queue_event(int event, unsigned long *pending,
-   struct sas_work *work,
+static void sas_queue_event(int event, struct sas_work *work,
struct sas_ha_struct *ha)
 {
-   if (!test_and_set_bit(event, pending)) {
-   unsigned long flags;
+   unsigned long flags;
 
-   spin_lock_irqsave(>lock, flags);
-   sas_queue_work(ha, work);
-   spin_unlock_irqrestore(>lock, flags);
-   }
+   spin_lock_irqsave(>lock, flags);
+   sas_queue_work(ha, work);
+   spin_unlock_irqrestore(>lock, flags);
 }
 
 
@@ -111,52 +112,87 @@ void sas_enable_revalidation(struct sas_ha_struct *ha)
if (!test_and_clear_bit(ev, >pending))
continue;
 
-   sas_queue_event(ev, >pending, >disc_work[ev].work, ha);
+   sas_queue_event(ev, >disc_work[ev].work, ha);
}
mutex_unlock(>disco_mutex);
 }
 
+static void sas_ha_event_worker(struct work_struct *work)
+{
+   struct sas_ha_event *ev = to_sas_ha_event(work);
+
+   sas_ha_event_fns[ev->type](work);
+   kfree(ev);
+}
+
+static void sas_port_event_worker(struct work_struct *work)
+{
+   struct asd_sas_event *ev = to_asd_sas_event(work);
+
+   sas_port_event_fns[ev->type](work);
+   kfree(ev);
+}
+
+static void sas_phy_event_worker(struct work_struct *work)
+{
+   struct asd_sas_event *ev = to_asd_sas_event(work);
+
+   sas_phy_event_fns[ev->type](work);
+   kfree(ev);
+}
+
 static void notify_ha_event(struct sas_ha_struct *sas_ha, enum ha_event event)
 {
+   struct sas_ha_event *ev;
+
BUG_ON(event >= HA_NUM_EVENTS);
 
-   sas_queue_event(event, _ha->pending,
-   _ha->ha_events[event].work, sas_ha);
+   ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
+   if (!ev)
+   return;
+
+   INIT_SAS_WORK(>work, sas_ha_event_worker);
+   ev->ha = sas_ha;
+   ev->type = event;
+   sas_queue_event(event, >work, sas_ha);
 }
 
 static void notify_port_event(struct asd_sas_phy *phy, enum port_event event)
 {
+   struct asd_sas_event *ev;
struct sas_ha_struct *ha = phy->ha;
 
BUG_ON(event >= PORT_NUM_EVENTS);
 
-   sas_queue_event(event, >port_events_pending,
-   >port_events[event].work, ha);
+   ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
+   if (!ev)
+   return;
+
+   INIT_SAS_WORK(>work, sas_port_event_worker);
+   ev->phy = phy;
+   ev->type = event;
+   sas_queue_event(event, >work, ha);
 }
 
 void sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event)
 {
+   struct asd_sas_event *ev;
struct sas_ha_struct *ha = phy->ha;
 
BUG_ON(event >= PHY_NUM_EVENTS);
 
-   sas_queue_event(event, >phy_events_pending,
-   >phy_events[event].work, ha);
+   ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
+   if (!ev)
+   return;
+
+   INIT_SAS_WORK(>work, sas_phy_event_worker);
+   ev->phy = phy;
+   ev->type = event;
+

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

Re: [PATCH v2 1/2] libsas: Don't process sas events in static works

[Resend][PATCH v2 1/2] libsas: Don't process sas events in static works

[Resend][PATCH v2 1/2] libsas: Don't process sas events in static works

[PATCH v2 1/2] libsas: Don't process sas events in static works

[PATCH v2 1/2] libsas: Don't process sas events in static works

20 matches

Site Navigation

Mail list logo

Footer information