Re: [PATCH 2/2] qla2xxx: remove irq save in qla2x00_poll()

[Sebastian Andrzej Siewior]

On 2018-06-11 16:56:35 [+0200], Johannes Thumshirn wrote:
> Looks good,
> Reviewed-by: Johannes Thumshirn 

Martin, what about this one?

Sebastian

Re: [PATCH 1/2 v2] libsas: remove irq save in sas_ata_qc_issue()

[Sebastian Andrzej Siewior]

On 2018-06-14 21:59:18 [-0400], Martin K. Petersen wrote:
> 
> Sebastian,
Martin,

> Applied to 4.19/scsi-queue. Thank you!

Thank you.

Sebastian

[PATCH 1/2 v2] libsas: remove irq save in sas_ata_qc_issue()

[Sebastian Andrzej Siewior]

Since commit 312d3e56119a ("[SCSI] libsas: remove ata_port.lock
management duties from lldds") the sas_ata_qc_issue() function unlocks
the ata_port.lock and disables interrupts before doing so.
That lock is always taken with disabled interrupts so at this point, the
interrupts are already disabled. There is no need to disable the
interrupts before the unlock operation because they are already
disabled.
Restoring the interrupt state later does not change anything because
they were disabled and remain disabled. Therefore remove the operations
which do not change the behaviour.

Signed-off-by: Sebastian Andrzej Siewior 
---
v1…v2: Updating comment as suggested by Dan Williams.

 drivers/scsi/libsas/sas_ata.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index ff1d612f6fb9..2ac7395112b4 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -176,7 +176,6 @@ static void sas_ata_task_done(struct sas_task *task)
 
 static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
 {
-   unsigned long flags;
struct sas_task *task;
struct scatterlist *sg;
int ret = AC_ERR_SYSTEM;
@@ -187,10 +186,7 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd 
*qc)
struct Scsi_Host *host = sas_ha->core.shost;
struct sas_internal *i = to_sas_internal(host->transportt);
 
-   /* TODO: audit callers to ensure they are ready for qc_issue to
-* unconditionally re-enable interrupts
-*/
-   local_irq_save(flags);
+   /* TODO: we should try to remove that unlock */
spin_unlock(ap->lock);
 
/* If the device fell off, no sense in issuing commands */
@@ -252,7 +248,6 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd 
*qc)
 
  out:
spin_lock(ap->lock);
-   local_irq_restore(flags);
return ret;
 }
 
-- 
2.17.1

Re: [PATCH 0/2 REPOST] remove unneded irq save

[Sebastian Andrzej Siewior]

On 2018-06-12 08:46:38 [-0700], Dan Williams wrote:
> On Tue, Jun 12, 2018 at 8:04 AM, John Garry  wrote:
> >> We had this comment for 6 years or so and nothing happend. What makes
> >> you think that an updated version of that comment will motivate someone
> >> to make change here in the near future?
> >
> > Updating the comment is not in itself something to motivate someone to
> > change, but we should keep the comment reasonably accurate or get rid.
> >
> >> It looks to me like a stale comment which won't change a thing because
> >> it does not point out the benefit of doing so (re-enabling interrupts
> >> while dropping the lock) and the price, that is paid for not doing so
> >> (keeping the code as it is) is small enough to not bother.
> >>
> >> So if updating the comment as suggested instead of keeping it as-is or
> >> removing it is the blocker *here* then I can send an updated version.
> >> Any comments?
> >
> >
> > I'd prefer an updated comment.
> >
> 
> I think we should try to remove the unlock completely. I agree with
> Sebastian that the audit is never coming. As it is libsas is the only
> ata_port_operations implementation that drops the host_lock while
> running ->qc_issue().

Dan, so in meantime I update the comment in patch #1 [0] to say "we
should try to remove unlock completely"?

[0] https://lkml.kernel.org/r/20180611144053.18294-2-bige...@linutronix.de

Sebastian

Re: [PATCH 0/2 REPOST] remove unneded irq save

[Sebastian Andrzej Siewior]

On 2018-06-12 13:54:36 [+0100], John Garry wrote:
> +Dan
> 
> On 11/06/2018 19:23, Sebastian Andrzej Siewior wrote:
> > On 2018-06-11 18:12:55 [+0100], John Garry wrote:
> > > On 11/06/2018 15:40, Sebastian Andrzej Siewior wrote:
> > > > This is the repost of the two patches I posted in earlier this month:
> > > > 
> > > > - [PATCH 1/2] libsas: remove irq save in sas_ata_qc_issue()
> > > >   Received feedback but nothing really changed. I explained that this is
> > > >   not about "local_irqsave() + spin_lock()" *but* "local_irq_save() +
> > > >   spin_unlock()". This seemed to have been overseen twice.
> > > >   Also there were two opinions about the TODO comment:
> > > >  /* TODO: audit callers to ensure they are ready for qc_issue to
> > > >   * unconditionally re-enable interrupts
> > > >It is unclear to me if this comment should be removed because it
> > > >makes no sense or if the intention was indeed to audit callers code
> > > >for a possible "spin_unlock_irq(ap->lock);".
> > > 
> > > Hi,
> > > 
> > > As I said previously, since it is not clear now what the comment meant, 
> > > then
> > > removing the irq save/restore calls will only make it even less clear, and
> > > should be fixed.
> > 
> > how should it be fixed? The discussion went forth and back. The comment
> > as of now does not match the code. It disables interrupts which are
> > already disabled. It does not enable them at any point.
> 
> As I see, at the point we release the lock, the question is if we can just
> re-enable interrupts as probably we disabled interrupts earlier for taking
> the same lock.
>
> However, as mentioned, we should not need to disable interrupts as they
> should have been already disabled.

The irq_save + unlock combo got first introduced in commit ce4f75def399
("isci: Free host lock for SATA/STP abort escalation at submission
time."). It then got moved forth and back until it ended where it is
today with the comment Dan put there. Back then the code path was:
- ata_exec_internal_sg()
  spin_lock_irqsave(ap->lock, flags);
  -> ata_qc_issue() (saying LOCKING:spin_lock_irqsave(host lock)
-> qc->err_mask |= ap->ops->qc_issue(qc); => sas_ata_qc_issue()
   -> i->dft->lldd_execute_task(task, 1, GFP_ATOMIC); => 
isci_task_execute_task()
 ->isci_request_execute(ihost, task, , gfp_flags);
   if (dev_is_sata(task->dev)) {
/* Since we are still in the submit path, and since
 * libsas takes the host lock on behalf of SATA
 * devices before I/O starts, we need to unlock
 * before we can put the task in the error path.
 */
 raw_local_irq_save(flags);
 spin_unlock(isci_host->shost->host_lock);
 sas_task_abort(task)

So. This when the mistake was introduced - it shouldn't get in there
like that.

> Personally I would rather not change the code. But, if you must, I suggest
of course.

> update the comment to read something like this:
> - TODO: It may be possible to unconditionally re-enable interrupts for
> period of having the lock released. Audit callers to check.

We had this comment for 6 years or so and nothing happend. What makes
you think that an updated version of that comment will motivate someone
to make change here in the near future?
It looks to me like a stale comment which won't change a thing because
it does not point out the benefit of doing so (re-enabling interrupts
while dropping the lock) and the price, that is paid for not doing so
(keeping the code as it is) is small enough to not bother.

So if updating the comment as suggested instead of keeping it as-is or
removing it is the blocker *here* then I can send an updated version.
Any comments?

> Cheers,
> John
 
Sebastian

Re: [PATCH 0/2 REPOST] remove unneded irq save

[Sebastian Andrzej Siewior]

On 2018-06-11 18:12:55 [+0100], John Garry wrote:
> On 11/06/2018 15:40, Sebastian Andrzej Siewior wrote:
> > This is the repost of the two patches I posted in earlier this month:
> > 
> > - [PATCH 1/2] libsas: remove irq save in sas_ata_qc_issue()
> >   Received feedback but nothing really changed. I explained that this is
> >   not about "local_irqsave() + spin_lock()" *but* "local_irq_save() +
> >   spin_unlock()". This seemed to have been overseen twice.
> >   Also there were two opinions about the TODO comment:
> >  /* TODO: audit callers to ensure they are ready for qc_issue to
> >   * unconditionally re-enable interrupts
> >It is unclear to me if this comment should be removed because it
> >makes no sense or if the intention was indeed to audit callers code
> >for a possible "spin_unlock_irq(ap->lock);".
> 
> Hi,
> 
> As I said previously, since it is not clear now what the comment meant, then
> removing the irq save/restore calls will only make it even less clear, and
> should be fixed.

how should it be fixed? The discussion went forth and back. The comment
as of now does not match the code. It disables interrupts which are
already disabled. It does not enable them at any point.

> Cheers,
> John

Sebastian

[PATCH 2/2] qla2xxx: remove irq save in qla2x00_poll()

[Sebastian Andrzej Siewior]

In commit d2ba5675d899 ("[SCSI] qla2xxx: Disable local-interrupts while
polling for RISC status.") added a local_irq_disable() before invoking
the ->intr_handler callback. The function, which was used in this
callback, did not disable interrupts while acquiring the spin_lock so a
deadlock was possible and this change was one possible solution.

The function in question was qla2300_intr_handler() and is using
spin_lock_irqsave() since commit 43fac4d97a1a ("[SCSI] qla2xxx: Resolve
a performance issue in interrupt").
I checked all other ->intr_handler callbacks and all of them use the
irqsave variant so it is safe to remove the local_irq_save() block now.

Cc: qla2xxx-upstr...@qlogic.com
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/scsi/qla2xxx/qla_inline.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_inline.h 
b/drivers/scsi/qla2xxx/qla_inline.h
index 37ae0f6d8ae5..bcbdf28bd7b9 100644
--- a/drivers/scsi/qla2xxx/qla_inline.h
+++ b/drivers/scsi/qla2xxx/qla_inline.h
@@ -58,14 +58,12 @@ qla2x00_debounce_register(volatile uint16_t __iomem *addr)
 static inline void
 qla2x00_poll(struct rsp_que *rsp)
 {
-   unsigned long flags;
struct qla_hw_data *ha = rsp->hw;
-   local_irq_save(flags);
+
if (IS_P3P_TYPE(ha))
qla82xx_poll(0, rsp);
else
ha->isp_ops->intr_handler(0, rsp);
-   local_irq_restore(flags);
 }
 
 static inline uint8_t *
-- 
2.17.1

[PATCH 1/2] libsas: remove irq save in sas_ata_qc_issue()

[Sebastian Andrzej Siewior]

Since commit 312d3e56119a ("[SCSI] libsas: remove ata_port.lock
management duties from lldds") the sas_ata_qc_issue() function unlocks
the ata_port.lock and disables interrupts before doing so.
That lock is always taken with disabled interrupts so at this point, the
interrupts are already disabled. There is no need to disable the
interrupts before the unlock operation because they are already
disabled.
Restoring the interrupt state later does not change anything because
they were disabled and remain disabled. Therefore remove the operations
which do not change the behaviour.

Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/scsi/libsas/sas_ata.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index ff1d612f6fb9..1216e6711178 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -176,7 +176,6 @@ static void sas_ata_task_done(struct sas_task *task)
 
 static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
 {
-   unsigned long flags;
struct sas_task *task;
struct scatterlist *sg;
int ret = AC_ERR_SYSTEM;
@@ -190,7 +189,6 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd 
*qc)
/* TODO: audit callers to ensure they are ready for qc_issue to
 * unconditionally re-enable interrupts
 */
-   local_irq_save(flags);
spin_unlock(ap->lock);
 
/* If the device fell off, no sense in issuing commands */
@@ -252,7 +250,6 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd 
*qc)
 
  out:
spin_lock(ap->lock);
-   local_irq_restore(flags);
return ret;
 }
 
-- 
2.17.1

[PATCH 0/2 REPOST] remove unneded irq save

[Sebastian Andrzej Siewior]

This is the repost of the two patches I posted in earlier this month:

- [PATCH 1/2] libsas: remove irq save in sas_ata_qc_issue()
  Received feedback but nothing really changed. I explained that this is
  not about "local_irqsave() + spin_lock()" *but* "local_irq_save() +
  spin_unlock()". This seemed to have been overseen twice.
  Also there were two opinions about the TODO comment:
 /* TODO: audit callers to ensure they are ready for qc_issue to
  * unconditionally re-enable interrupts
   It is unclear to me if this comment should be removed because it
   makes no sense or if the intention was indeed to audit callers code
   for a possible "spin_unlock_irq(ap->lock);".

- [PATCH 2/2] qla2xxx: remove irq save in qla2x00_poll()
  Received no feedback.

Sebastian

Re: [PATCH 1/2] libsas: remove irq save in sas_ata_qc_issue()

[Sebastian Andrzej Siewior]

On 2018-05-30 15:08:37 [+0100], John Garry wrote:
> On 30/05/2018 14:45, Sebastian Andrzej Siewior wrote:
> > excellent. So no more objections from your side or is this a complaint I
> > didn't fully decode?
> 
> I think the original code is not great since we're dropping the lock but
> maintaining the irq posture as disabled.

the patch does not change the locking as it exists today.

> Do you plan to add a lockdep_assert_irqs_disabled() check in addition? It's
> not needed if we work on the basis that the lock is always taken with irqs
> disabled. And is this lockdep function even intended to be used outside core
> kernel code?

I don't plan to add lockdep_assert_irqs_disabled(). You should only need
to use lockdep_assert_held() to verify that the lock is held as
expected. Whether or not the interrupts need to be disabled is best
verified with lockdep. Lockdep would even complain here if you intend to
invoke spin_unlock() on a lock which was not acquired earlier (I'm just
trying to say that lockdep_assert_held() is not required here either).

> Personally I think that solving the issue in the original code would be
> better, i.e. keeping irqs disabled needlessly. Or else the "TODO" can stay
> (or be amended to improve clarity).

I would like to get rid of that local_irq_save() here because it breaks
-RT and is not the correct thing to do for !RT (but it does not break
things).
I posted v1 with just the removal of the local_irq_sav() and v2 with
TODO removed as suggested here in the thread. The interrupts still
remain disabled.
So based on that it seems that you are in favour of the initial v1.

> John

Sebastian

Re: [PATCH 1/2] libsas: remove irq save in sas_ata_qc_issue()

[Sebastian Andrzej Siewior]

On 2018-05-30 14:37:50 [+0100], John Garry wrote:
> On 30/05/2018 12:22, Sebastian Andrzej Siewior wrote:
> > Yes, there are the same on vanilla and not on RT. However my point is
> > that the code does this instead:
> > local_irq_save();
> > spin_unlock();
> 
> Ah, I just noticed that this is spin_unlock().
> 
> So about the "TODO", which you mention "I *assumed* that the intention was
> to audit the code for this
> 
>   spin_unlock_irq(ap->lock);
> 
> change instead. But if this is or was never intended than I could indeed
> remove the TODO comment."
> 
> As I see, we're dropping the lock but maintaining the irq posture for
> holding that lock (disabled), which seems inefficient.

excellent. So no more objections from your side or is this a complaint I
didn't fully decode?

> John
> 
Sebastian

Re: [PATCH 1/2] libsas: remove irq save in sas_ata_qc_issue()

[Sebastian Andrzej Siewior]

On 2018-05-30 12:16:23 [+0100], John Garry wrote:
> On 30/05/2018 11:16, Sebastian Andrzej Siewior wrote:
> > On 2018-05-30 10:34:12 [+0100], John Garry wrote:
> > > Sorry, but personally I don't see much value in this change. I think it's
> > > better for safety to be consistent in how we lock & unlock the spinlock,
> > > i.e. use irqsave variant (or similar).
> > 
> > The lock should do irqsave() and unlock irqrestore(). This
> > local_irqsave() + unlock() is not correct.
> > 
> 
> Aren't they the same, i.e. local_irq_save()+spin_lock() =
> spin_lock_irqsave()? Both give state of lock held, interrupts and preemption
> disabled.

Yes, there are the same on vanilla and not on RT. However my point is
that the code does this instead:
local_irq_save();
spin_unlock();

and this is wrong. There is no spin_unlock_irqsave().

> 
> John
> 
Sebastian

Re: [PATCH 1/2] libsas: remove irq save in sas_ata_qc_issue()

[Sebastian Andrzej Siewior]

On 2018-05-30 10:34:12 [+0100], John Garry wrote:
> Sorry, but personally I don't see much value in this change. I think it's
> better for safety to be consistent in how we lock & unlock the spinlock,
> i.e. use irqsave variant (or similar).

The lock should do irqsave() and unlock irqrestore(). This
local_irqsave() + unlock() is not correct.

> And even if we have audited the callers to know the irqstate when this
> function is called, since sas_ata_qc_issue() is a callback for
> ata_port_operations.qc_issue, then it should be documented that interrupts
> should always be disabled for this callback.

If the lock has been acquired without disabling the interrupts then it
was already wrong. This is just duct tape and does not fix the
problematic caller. Lockdep will yell.

> All the best,
> John

Sebastian

[PATCH 1/2 v2] scsi: libsas: remove irq save in sas_ata_qc_issue()

[Sebastian Andrzej Siewior]

Since commit 312d3e56119a ("[SCSI] libsas: remove ata_port.lock
management duties from lldds") the sas_ata_qc_issue() function unlocks
the ata_port.lock and disables interrupts before doing so.
That lock is always taken with disabled interrupts so at this point, the
interrupts are already disabled. There is no need to disable the
interrupts before the unlock operation because they are already
disabled.
Restoring the interrupt state later does not change anything because
they were disabled and remain disabled. Therefore remove the operations
which do not change the behaviour.

Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
v1…v2: Drop the TODO comment as suggested by Jason Yan.

 drivers/scsi/libsas/sas_ata.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 0cc1567eacc1..b3be3935b831 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -176,7 +176,6 @@ static void sas_ata_task_done(struct sas_task *task)
 
 static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
 {
-   unsigned long flags;
struct sas_task *task;
struct scatterlist *sg;
int ret = AC_ERR_SYSTEM;
@@ -187,10 +186,6 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd 
*qc)
struct Scsi_Host *host = sas_ha->core.shost;
struct sas_internal *i = to_sas_internal(host->transportt);
 
-   /* TODO: audit callers to ensure they are ready for qc_issue to
-* unconditionally re-enable interrupts
-*/
-   local_irq_save(flags);
spin_unlock(ap->lock);
 
/* If the device fell off, no sense in issuing commands */
@@ -252,7 +247,6 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd 
*qc)
 
  out:
spin_lock(ap->lock);
-   local_irq_restore(flags);
return ret;
 }
 
-- 
2.17.0

Re: [PATCH 1/2] libsas: remove irq save in sas_ata_qc_issue()

[Sebastian Andrzej Siewior]

On 2018-05-24 16:20:33 [+0800], Jason Yan wrote:
> NO, this:
> 
>   /* TODO: audit callers to ensure they are ready for qc_issue to
>* unconditionally re-enable interrupts
>*/
>   local_irq_save(flags);
>   spin_unlock(ap->lock);

indeed, I have no idea how I could have overseen this. At least as of
today, the interrupts are never "re-enabled" in this function. I
*assumed* that the intention was to audit the code for this

spin_unlock_irq(ap->lock);

change instead. But if this is or was never intended than I could indeed
remove the TODO comment.

Sebastian

Re: [PATCH 1/2] libsas: remove irq save in sas_ata_qc_issue()

[Sebastian Andrzej Siewior]

On 2018-05-24 16:00:47 [+0800], Jason Yan wrote:
> > > Because you are trying to delete the irq-save code which will make the
> > > comment hard to understand, I'm just giving an advise. It's welcome
> > > if you have a better way.
> > 
> > I am removing local_irq_save() prior an unlock of a lock which has to be
> > taken with disabled interrupts. Therefore the interrupts are disabling
> > as part of the locking procedure and the additional disabling is a nop.
> > 
> > What comment are you talking about? sas_ata_qc_issue() has no comments
> > except for "If the device fell …".
> > 
> 
> I mean the "TODO" comment above the local_irq_save().
this?
/*
 * The local_irq_*() APIs are equal to the raw_local_irq*()
 * if !TRACE_IRQFLAGS.
 */

Sebastian

Re: [PATCH 1/2] libsas: remove irq save in sas_ata_qc_issue()

[Sebastian Andrzej Siewior]

On 2018-05-24 09:50:07 [+0200], Johannes Thumshirn wrote:
> > lockdep_assert_held() cannot detect the irq state, it can only detect
> > whether we have held the lock.
> 
> I think Sebastian wanted to say lockdep_assert_irqs_disabled().

nope, meant the right thing.

> Byte,
>   Johannes

Sebastian

Re: [PATCH 1/2] libsas: remove irq save in sas_ata_qc_issue()

[Sebastian Andrzej Siewior]

On 2018-05-24 15:38:51 [+0800], Jason Yan wrote:
> > no, please don't do this. Please add instead
> > lockdep_assert_held()
> > 
> > on the lock in question and let lockdep to its work. Lockdep has way
> > better coverage than your irqs_disabled() check which also breaks RT.
> > 
> 
> lockdep_assert_held() cannot detect the irq state, it can only detect
> whether we have held the lock.

yes, correct. But if the lock is acquired with interrupts enabled or the
interrupts are enabled while the lock is held then lockdep will notify
you. So you ensure that the lock is held at this point then and lockdep
will do the validation (however, no need to do so in sas_ata_qc_issue()
because lockdep do the evaluation during unlock/lock).

> Because you are trying to delete the irq-save code which will make the
> comment hard to understand, I'm just giving an advise. It's welcome
> if you have a better way.

I am removing local_irq_save() prior an unlock of a lock which has to be
taken with disabled interrupts. Therefore the interrupts are disabling
as part of the locking procedure and the additional disabling is a nop.

What comment are you talking about? sas_ata_qc_issue() has no comments
except for "If the device fell …".

> Thanks.
> 
Sebastian

Re: [PATCH 1/2] libsas: remove irq save in sas_ata_qc_issue()

[Sebastian Andrzej Siewior]

On 2018-05-24 10:58:44 [+0800], Jason Yan wrote:
> I think it's fine to delete this irq save code. As for the "TODO"
> comment, I think we can add:
> 
> BUG_ON(!irqs_disabled());
> 
> or
> 
> WARN_ON_ONCE(!irqs_disabled());

no, please don't do this. Please add instead
lockdep_assert_held()

on the lock in question and let lockdep to its work. Lockdep has way
better coverage than your irqs_disabled() check which also breaks RT.

> and then delete the "TODO" comment.

Sebastian

Re: [PATCH 1/2] libsas: remove irq save in sas_ata_qc_issue()

[Sebastian Andrzej Siewior]

On 2018-05-23 09:46:30 [+0100], John Garry wrote:
> On 22/05/2018 18:31, Sebastian Andrzej Siewior wrote:
> > On 2018-05-18 14:31:27 [+0100], John Garry wrote:
> > > On 04/05/2018 15:50, Sebastian Andrzej Siewior wrote:
> > > > Since commit 312d3e56119a ("[SCSI] libsas: remove ata_port.lock
> > > > management duties from lldds") the sas_ata_qc_issue() function unlocks
> > > > the ata_port.lock and disables interrupts before doing so.
> > > > That lock is always taken with disabled interrupts so at this point, the
> > > > interrupts are already disabled. There is no need to disable the
> > > > interrupts before the unlock operation because they are already
> > > > disabled.
> > > 
> > > Is the only caller ata_qc_issue(), which has spin_lock_irqsave(host lock)
> > > enabled?
> > 
> > I don't understand the question.
> 
> It seems to me that ap->lock can be taken in interrupt context, so then we
> know it should be locked with interrupts disabled always.

yes, the comment above the function says so, too.

> I was just really asking how you know interrupts are disabled already? Maybe
> it's obvious.

The lock has to be taken with interrupts disabled. If the interrupts
were enabled at the time sas_ata_qc_issue() was invoked then the lock
was already taken in a bad way and disabling interrupts before the
unlock does not make it any better.
To verify that the locking is okay you can build the kernel with lockdep
enabled and CONFIG_PROVE_LOCKING=y set. That way lockdep will inform you
as soon as it notices the lock taken in interrupt context and in
process-context with enabled interrupts.

> cheers,

Sebastian

Re: [PATCH 1/2] libsas: remove irq save in sas_ata_qc_issue()

[Sebastian Andrzej Siewior]

On 2018-05-18 14:31:27 [+0100], John Garry wrote:
> On 04/05/2018 15:50, Sebastian Andrzej Siewior wrote:
> > Since commit 312d3e56119a ("[SCSI] libsas: remove ata_port.lock
> > management duties from lldds") the sas_ata_qc_issue() function unlocks
> > the ata_port.lock and disables interrupts before doing so.
> > That lock is always taken with disabled interrupts so at this point, the
> > interrupts are already disabled. There is no need to disable the
> > interrupts before the unlock operation because they are already
> > disabled.
> 
> Is the only caller ata_qc_issue(), which has spin_lock_irqsave(host lock)
> enabled?

I don't understand the question. 

> > Restoring the interrupt state later does not change anything because
> > they were disabled and remain disabled. Therefore remove the operations
> > which do not change the behaviour.
> > 
> > Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
> > ---
> >  drivers/scsi/libsas/sas_ata.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> > index 0cc1567eacc1..bc5d917ff643 100644
> > --- a/drivers/scsi/libsas/sas_ata.c
> > +++ b/drivers/scsi/libsas/sas_ata.c
> > @@ -176,7 +176,6 @@ static void sas_ata_task_done(struct sas_task *task)
> > 
> >  static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
> >  {
> > -   unsigned long flags;
> > struct sas_task *task;
> > struct scatterlist *sg;
> > int ret = AC_ERR_SYSTEM;
> > @@ -190,7 +189,6 @@ static unsigned int sas_ata_qc_issue(struct 
> > ata_queued_cmd *qc)
> > /* TODO: audit callers to ensure they are ready for qc_issue to
> >  * unconditionally re-enable interrupts
> >  */
> 
> Is the "TODO" comment still relevant?

can't tell. The interrupts are never enabled during the unlock.

> cheers,
> 
> > -   local_irq_save(flags);
> > spin_unlock(ap->lock);
> > 
> > /* If the device fell off, no sense in issuing commands */
> > @@ -252,7 +250,6 @@ static unsigned int sas_ata_qc_issue(struct 
> > ata_queued_cmd *qc)
> > 
> >   out:
> > spin_lock(ap->lock);
> > -   local_irq_restore(flags);
> > return ret;
> >  }

Sebastian

[PATCH 2/2] qla2xxx: remove irq save in qla2x00_poll()

[Sebastian Andrzej Siewior]

In commit d2ba5675d899 ("[SCSI] qla2xxx: Disable local-interrupts while
polling for RISC status.") added a local_irq_disable() before invoking
the ->intr_handler callback. The function, which was used in this
callback, did not disable interrupts while acquiring the spin_lock so a
deadlock was possible and this change was one possible solution.

The function in question was qla2300_intr_handler() and is using
spin_lock_irqsave() since commit 43fac4d97a1a ("[SCSI] qla2xxx: Resolve
a performance issue in interrupt").
I checked all other ->intr_handler callbacks and all of them use the
irqsave variant so it is safe to remove the local_irq_save() block now.

Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/qla2xxx/qla_inline.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_inline.h 
b/drivers/scsi/qla2xxx/qla_inline.h
index 37ae0f6d8ae5..bcbdf28bd7b9 100644
--- a/drivers/scsi/qla2xxx/qla_inline.h
+++ b/drivers/scsi/qla2xxx/qla_inline.h
@@ -58,14 +58,12 @@ qla2x00_debounce_register(volatile uint16_t __iomem *addr)
 static inline void
 qla2x00_poll(struct rsp_que *rsp)
 {
-   unsigned long flags;
struct qla_hw_data *ha = rsp->hw;
-   local_irq_save(flags);
+
if (IS_P3P_TYPE(ha))
qla82xx_poll(0, rsp);
else
ha->isp_ops->intr_handler(0, rsp);
-   local_irq_restore(flags);
 }
 
 static inline uint8_t *
-- 
2.17.0

[PATCH 1/2] libsas: remove irq save in sas_ata_qc_issue()

[Sebastian Andrzej Siewior]

Since commit 312d3e56119a ("[SCSI] libsas: remove ata_port.lock
management duties from lldds") the sas_ata_qc_issue() function unlocks
the ata_port.lock and disables interrupts before doing so.
That lock is always taken with disabled interrupts so at this point, the
interrupts are already disabled. There is no need to disable the
interrupts before the unlock operation because they are already
disabled.
Restoring the interrupt state later does not change anything because
they were disabled and remain disabled. Therefore remove the operations
which do not change the behaviour.

Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/libsas/sas_ata.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 0cc1567eacc1..bc5d917ff643 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -176,7 +176,6 @@ static void sas_ata_task_done(struct sas_task *task)
 
 static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
 {
-   unsigned long flags;
struct sas_task *task;
struct scatterlist *sg;
int ret = AC_ERR_SYSTEM;
@@ -190,7 +189,6 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd 
*qc)
/* TODO: audit callers to ensure they are ready for qc_issue to
 * unconditionally re-enable interrupts
 */
-   local_irq_save(flags);
spin_unlock(ap->lock);
 
/* If the device fell off, no sense in issuing commands */
@@ -252,7 +250,6 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd 
*qc)
 
  out:
spin_lock(ap->lock);
-   local_irq_restore(flags);
return ret;
 }
 
-- 
2.17.0

[PATCH 1/2 v2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[Sebastian Andrzej Siewior]

There are a few functions which check for if the lock is held
(spin_lock_assert()) and the interrupts are disabled (irqs_disabled()).
>From looking at the code, each function is static, the caller is near by
and does spin_lock_irq|safe(). As Linus puts it:

|It's not like this is some function that is exported to random users,
|and we should check that the calling convention is right.
|
|This looks like "it may have been useful during coding to document
|things, but it's not useful long-term".

Remove those checks.

Reported-by: Arnaldo Carvalho de Melo <a...@kernel.org>
Suggested-by: Linus Torvalds <torva...@linux-foundation.org>
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
v1…2: Add credits.

 drivers/target/target_core_tmr.c   | 2 --
 drivers/target/target_core_transport.c | 6 --
 2 files changed, 8 deletions(-)

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 9c7bc1ca341a..3d35dad1de2c 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -114,8 +114,6 @@ static bool __target_check_io_state(struct se_cmd *se_cmd,
 {
struct se_session *sess = se_cmd->se_sess;
 
-   assert_spin_locked(>sess_cmd_lock);
-   WARN_ON_ONCE(!irqs_disabled());
/*
 * If command already reached CMD_T_COMPLETE state within
 * target_complete_cmd() or CMD_T_FABRIC_STOP due to shutdown,
diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index 74b646f165d4..2c3ddb3a4124 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2954,9 +2954,6 @@ __transport_wait_for_tasks(struct se_cmd *cmd, bool 
fabric_stop,
__acquires(>t_state_lock)
 {
 
-   assert_spin_locked(>t_state_lock);
-   WARN_ON_ONCE(!irqs_disabled());
-
if (fabric_stop)
cmd->transport_state |= CMD_T_FABRIC_STOP;
 
@@ -3241,9 +3238,6 @@ static int __transport_check_aborted_status(struct se_cmd 
*cmd, int send_status)
 {
int ret;
 
-   assert_spin_locked(>t_state_lock);
-   WARN_ON_ONCE(!irqs_disabled());
-
if (!(cmd->transport_state & CMD_T_ABORTED))
return 0;
/*
-- 
2.16.2

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Sebastian Andrzej Siewior]

On 2018-03-22 06:37:45 [-0300], Arnaldo Carvalho de Melo wrote:
> Em Wed, Mar 21, 2018 at 11:43:58AM -0700, Linus Torvalds escreveu:
> > [ Adding PeterZ to participants due to query about lockdep_assert() ]
> > 
> > On Wed, Mar 21, 2018 at 8:38 AM, Arnaldo Carvalho de Melo
> >  wrote:
> > >
> > > assert_spin_locked(>t_state_lock);
> > > -   WARN_ON_ONCE(!irqs_disabled());
> > > +   WARN_ON_ONCE_NONRT(!irqs_disabled());
> > 
> > Ugh.
> > 
> > Can't we just replace both of those with a lockdep annotation?
> 
> Huh, even better, when that feature gets finished (tglx said it was
> being developed, not there yet tho) it'll allow further reduction of the
> PREEMPT_RT_FULL patchkit.

I am going take this into -RT tree for now until we have different
solution. I will try to be kind and do the same change in
__transport_wait_for_tasks().
Arnaldo, please do "[PATCH RT]" while sending patches. Then the bots
don't complain if it applies but does not compile on !RT kernel (or so
I've been told).

Technically speaking the code wants to ensure that the lock is held and
the interrupts are disabled because the lock is always taken with
disabled interrupts. This kind of check could be done with
lockdep_assert_held(>t_state_lock);

but would require lockdep to be switched on. Nicholas, would you mind
such a change?

> - Arnaldo

Sebastian

Re: [PATCH 1/5] scsi: bnx2i: convert to workqueue

[Sebastian Andrzej Siewior]

On 2017-07-07 09:20:02 [-0400], Chad Dupuis wrote:
> What was the question?  My observation is that the patch I proposed fixed 
> the issue we saw on testing the patch set.  With that small change 
> (essentially modulo by the number of active CPUs vs. the total number) 
> your patch set worked ok.

That mail at the bottom of this mail where I said why I think your patch
is a nop in this context.

Sebastian

On 2017-05-17 17:07:34 [+0200], To Chad Dupuis wrote:
> > > Sebastian, can you add this change to your patch set?
> >
> > Are sure that you can reliably reproduce the issue and fix it with the
> > patch above? Because this patch:
>
> oh. Okay. Now it clicked. It can fix the issue but it is still possible,
> that CPU0 goes down between your check for it and schedule_work_on()
> returning. Let my think of something…

Oh wait. I already thought about this: it may take bnx2fc_percpu from
CPU7 and run the worker on CPU3. The job isn't lost, because the worker
does:

| static void bnx2fc_percpu_io_work(struct work_struct *work_s)
| {
| struct bnx2fc_percpu_s *p;
 …
| p = container_of(work_s, struct bnx2fc_percpu_s, work);
|
| spin_lock_bh(>fp_work_lock);

and so will access bnx2fc_percpu of CPU7 running on CPU3. So I *think*
that your patch should make no difference and there should be no leak if
schedule_work_on() is invoked on an offline CPU.

Re: [PATCH 1/5] scsi: bnx2i: convert to workqueue

[Sebastian Andrzej Siewior]

On 2017-06-29 15:57:56 [+0200], Johannes Thumshirn wrote:
> So here we are again,
> Tested-by: Johannes Thumshirn 
> 
> FCoE will follow as soon as my setup can speak FCoE again.

So it all looks good, doesn't it? Chad never responded to my question
on his patch. I still doubt that it fixes the problem he observed.

Sebastian

Re: [PREEMPT-RT] [REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

[Sebastian Andrzej Siewior]

On 2017-05-17 17:07:34 [+0200], To Chad Dupuis wrote:
> > > Sebastian, can you add this change to your patch set?
> > 
> > Are sure that you can reliably reproduce the issue and fix it with the
> > patch above? Because this patch:
> 
> oh. Okay. Now it clicked. It can fix the issue but it is still possible,
> that CPU0 goes down between your check for it and schedule_work_on()
> returning. Let my think of something…

Oh wait. I already thought about this: it may take bnx2fc_percpu from
CPU7 and run the worker on CPU3. The job isn't lost, because the worker
does:

| static void bnx2fc_percpu_io_work(struct work_struct *work_s)
| {
| struct bnx2fc_percpu_s *p;
 …
| p = container_of(work_s, struct bnx2fc_percpu_s, work);
| 
| spin_lock_bh(>fp_work_lock);

and so will access bnx2fc_percpu of CPU7 running on CPU3. So I *think*
that your patch should make no difference and there should be no leak if
schedule_work_on() is invoked on an offline CPU.
 
Sebastian

Re: [PREEMPT-RT] [REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

[Sebastian Andrzej Siewior]

On 2017-05-17 17:01:53 [+0200], To Chad Dupuis wrote:
> On 2017-05-12 11:55:52 [-0400], Chad Dupuis wrote:
> > Ok, I believe I've found the issue here.  The machine that the test has 
> > performed on had many more possible CPUs than active CPUs.  We calculate 
> > which CPU to the work time on in bnx2fc_process_new_cqes() like this:
> > 
> > unsigned int cpu = wqe % num_possible_cpus();
> > 
> > Since not all CPUs are active, we were trying to schedule work on 
> > non-active CPUs which meant that the upper layers were never notified of 
> > the completion.  With this change:
> > 
> > diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c 
> > b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> > index c2288d6..6f08e43 100644
> > --- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> > +++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> > @@ -1042,7 +1042,12 @@ static int bnx2fc_process_new_cqes(struct 
> > bnx2fc_rport *tgt)
> > /* Pending work request completion */
> > struct bnx2fc_work *work = NULL;
> > struct bnx2fc_percpu_s *fps = NULL;
> > -   unsigned int cpu = wqe % num_possible_cpus();
> > +   unsigned int cpu = wqe % num_active_cpus();
> > +
> > +   /* Sanity check cpu to make sure it's online */
> > +   if (!cpu_active(cpu))
> > +   /* Default to CPU 0 */
> > +   cpu = 0;
> >  
> > work = bnx2fc_alloc_work(tgt, wqe);
> > if (work) {
> > 
> > The issue is fixed.
> > 
> > Sebastian, can you add this change to your patch set?
> 
> Are sure that you can reliably reproduce the issue and fix it with the
> patch above? Because this patch:

oh. Okay. Now it clicked. It can fix the issue but it is still possible,
that CPU0 goes down between your check for it and schedule_work_on()
returning. Let my think of something…

Sebastian

Re: [REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

[Sebastian Andrzej Siewior]

On 2017-05-12 11:55:52 [-0400], Chad Dupuis wrote:
> Ok, I believe I've found the issue here.  The machine that the test has 
> performed on had many more possible CPUs than active CPUs.  We calculate 
> which CPU to the work time on in bnx2fc_process_new_cqes() like this:
> 
> unsigned int cpu = wqe % num_possible_cpus();
> 
> Since not all CPUs are active, we were trying to schedule work on 
> non-active CPUs which meant that the upper layers were never notified of 
> the completion.  With this change:
> 
> diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c 
> b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> index c2288d6..6f08e43 100644
> --- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> +++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> @@ -1042,7 +1042,12 @@ static int bnx2fc_process_new_cqes(struct 
> bnx2fc_rport *tgt)
> /* Pending work request completion */
> struct bnx2fc_work *work = NULL;
> struct bnx2fc_percpu_s *fps = NULL;
> -   unsigned int cpu = wqe % num_possible_cpus();
> +   unsigned int cpu = wqe % num_active_cpus();
> +
> +   /* Sanity check cpu to make sure it's online */
> +   if (!cpu_active(cpu))
> +   /* Default to CPU 0 */
> +   cpu = 0;
>  
> work = bnx2fc_alloc_work(tgt, wqe);
> if (work) {
> 
> The issue is fixed.
> 
> Sebastian, can you add this change to your patch set?

Are sure that you can reliably reproduce the issue and fix it with the
patch above? Because this patch:

diff --git a/init/main.c b/init/main.c
index b0c11cbf5ddf..483a971b1fd2 100644
--- a/init/main.c
+++ b/init/main.c
@@ -997,6 +997,12 @@ static int __ref kernel_init(void *unused)
  "See Linux Documentation/admin-guide/init.rst for guidance.");
 }
 
+static void workfn(struct work_struct *work)
+{
+   pr_err("%s() %d\n", __func__, raw_smp_processor_id());
+}
+static DECLARE_WORK(work, workfn);
+
 static noinline void __init kernel_init_freeable(void)
 {
/*
@@ -1040,6 +1046,15 @@ static noinline void __init kernel_init_freeable(void)
 
(void) sys_dup(0);
(void) sys_dup(0);
+   {
+
+   cpu_down(3);
+   pr_err("%s() num possible: %d\n", __func__, 
num_possible_cpus());
+   pr_err("%s() num online  : %d\n", __func__, num_online_cpus());
+   pr_err("%s() 3 active: %d\n", __func__, cpu_active(3));
+   schedule_work_on(3, );
+   ssleep(5);
+   }
/*
 * check if there is an early userspace init.  If yes, let it do all
 * the work

produces this output:
[1.960313] Unregister pv shared memory for cpu 3
[1.997000] kernel_init_freeable() num possible: 8
[1.998073] kernel_init_freeable() num online  : 7
[1.999125] kernel_init_freeable() 3 active: 0
[2.000337] workfn() 1

which means, CPU3 is offline and work runs on CPU1 instead. So it does
already what you suggest except that chances are, that it is not run on
CPU0 in this case (but on another CPU).

So it either takes some time for wait_for_completion(_req->tm_done);
to come back _or_ there is a leak somewhere where a complete() is
somehow missing / racing against something.

Sebastian

Re: [REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

[Sebastian Andrzej Siewior]

On 2017-04-10 19:12:49 [+0200], To Martin K . Petersen wrote:
> This is a repost to get the patches applied against v4.11-rc6. mkp's scsi
> for-next tree can be merged with no conflicts.
…

Martin, do you see any chance to get this merged? Chad replied to the
list that he is going to test it on 2017-04-10, didn't respond to the
ping 10 days later. The series stalled last time in the same way.

> The whole series is also available at
>  git://git.kernel.org/pub/scm/linux/kernel/git/bigeasy/hotplug-staging.git 
> scsi/bnx2_v2
 
Sebastian

Re: [REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

[Sebastian Andrzej Siewior]

On 2017-04-10 14:20:41 [-0400], Chad Dupuis wrote:
> Sebastian, will take a look. 

This [0] commit in tip's smp/hotplug branch is staged for the next merge
window. It will trigger a lockdep warning on the recursive
get_online_cpus() invocation in the two drivers. It is fixed/avoided by
the series.

[0] https://git.kernel.org/tip/tip/c/d215aab82d81974f438bfbc80aa437132f3c37c3

Sebastian

[PATCH 3/5] scsi: bnx2fc: clean up header definitions

[Sebastian Andrzej Siewior]

- All symbols which are only used within one .c file are marked static
  and removed from the bnx2fc.h file if possible.

- the declarion of bnx2fc_percpu is moved into the header file

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc.h  |  5 +
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 19 +--
 drivers/scsi/bnx2fc/bnx2fc_hwi.c  |  6 ++
 3 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc.h b/drivers/scsi/bnx2fc/bnx2fc.h
index 0279cc8de7a0..5b2151e7d894 100644
--- a/drivers/scsi/bnx2fc/bnx2fc.h
+++ b/drivers/scsi/bnx2fc/bnx2fc.h
@@ -172,6 +172,7 @@ struct bnx2fc_percpu_s {
struct list_head work_list;
spinlock_t fp_work_lock;
 };
+DECLARE_PER_CPU(struct bnx2fc_percpu_s, bnx2fc_percpu);
 
 struct bnx2fc_fw_stats {
u64 fc_crc_cnt;
@@ -513,7 +514,6 @@ void bnx2fc_cmd_mgr_free(struct bnx2fc_cmd_mgr *cmgr);
 void bnx2fc_get_link_state(struct bnx2fc_hba *hba);
 char *bnx2fc_get_next_rqe(struct bnx2fc_rport *tgt, u8 num_items);
 void bnx2fc_return_rqe(struct bnx2fc_rport *tgt, u8 num_items);
-int bnx2fc_get_paged_crc_eof(struct sk_buff *skb, int tlen);
 int bnx2fc_send_rrq(struct bnx2fc_cmd *aborted_io_req);
 int bnx2fc_send_adisc(struct bnx2fc_rport *tgt, struct fc_frame *fp);
 int bnx2fc_send_logo(struct bnx2fc_rport *tgt, struct fc_frame *fp);
@@ -537,7 +537,6 @@ void bnx2fc_init_task(struct bnx2fc_cmd *io_req,
 void bnx2fc_add_2_sq(struct bnx2fc_rport *tgt, u16 xid);
 void bnx2fc_ring_doorbell(struct bnx2fc_rport *tgt);
 int bnx2fc_eh_abort(struct scsi_cmnd *sc_cmd);
-int bnx2fc_eh_host_reset(struct scsi_cmnd *sc_cmd);
 int bnx2fc_eh_target_reset(struct scsi_cmnd *sc_cmd);
 int bnx2fc_eh_device_reset(struct scsi_cmnd *sc_cmd);
 void bnx2fc_rport_event_handler(struct fc_lport *lport,
@@ -570,8 +569,6 @@ struct fc_seq *bnx2fc_elsct_send(struct fc_lport *lport, 
u32 did,
   struct fc_frame *,
   void *),
  void *arg, u32 timeout);
-void bnx2fc_arm_cq(struct bnx2fc_rport *tgt);
-int bnx2fc_process_new_cqes(struct bnx2fc_rport *tgt);
 void bnx2fc_process_cq_compl(struct bnx2fc_rport *tgt, u16 wqe);
 struct bnx2fc_rport *bnx2fc_tgt_lookup(struct fcoe_port *port,
 u32 port_id);
diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c 
b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index 329922d51f8a..2f66c2ea093c 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -49,7 +49,7 @@ struct workqueue_struct *bnx2fc_wq;
  * Here the io threads are per cpu but the l2 thread is just one
  */
 struct fcoe_percpu_s bnx2fc_global;
-DEFINE_SPINLOCK(bnx2fc_global_lock);
+static DEFINE_SPINLOCK(bnx2fc_global_lock);
 
 static struct cnic_ulp_ops bnx2fc_cnic_cb;
 static struct libfc_function_template bnx2fc_libfc_fcn_templ;
@@ -107,22 +107,22 @@ MODULE_PARM_DESC(debug_logging,
"\t\t0x10 - fcoe L2 fame related logs.\n"
"\t\t0xff - LOG all messages.");
 
-uint bnx2fc_devloss_tmo;
+static uint bnx2fc_devloss_tmo;
 module_param_named(devloss_tmo, bnx2fc_devloss_tmo, uint, S_IRUGO);
 MODULE_PARM_DESC(devloss_tmo, " Change devloss_tmo for the remote ports "
"attached via bnx2fc.");
 
-uint bnx2fc_max_luns = BNX2FC_MAX_LUN;
+static uint bnx2fc_max_luns = BNX2FC_MAX_LUN;
 module_param_named(max_luns, bnx2fc_max_luns, uint, S_IRUGO);
 MODULE_PARM_DESC(max_luns, " Change the default max_lun per SCSI host. Default 
"
"0x.");
 
-uint bnx2fc_queue_depth;
+static uint bnx2fc_queue_depth;
 module_param_named(queue_depth, bnx2fc_queue_depth, uint, S_IRUGO);
 MODULE_PARM_DESC(queue_depth, " Change the default queue depth of SCSI devices 
"
"attached via bnx2fc.");
 
-uint bnx2fc_log_fka;
+static uint bnx2fc_log_fka;
 module_param_named(log_fka, bnx2fc_log_fka, uint, S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(log_fka, " Print message to kernel log when fcoe is "
"initiating a FIP keep alive when debug logging is enabled.");
@@ -167,7 +167,7 @@ static void bnx2fc_clean_rx_queue(struct fc_lport *lp)
spin_unlock_bh(>fcoe_rx_list.lock);
 }
 
-int bnx2fc_get_paged_crc_eof(struct sk_buff *skb, int tlen)
+static int bnx2fc_get_paged_crc_eof(struct sk_buff *skb, int tlen)
 {
int rc;
spin_lock(_global_lock);
@@ -1395,10 +1395,9 @@ static struct bnx2fc_hba *bnx2fc_hba_create(struct 
cnic_dev *cnic)
return NULL;
 }
 
-static struct bnx2fc_interface *
-bnx2fc_interface_create(struct bnx2fc_hba *hba,
-   struct net_device *netdev,
-   enum fip_state fip_mode)
+static st

[PATCH 5/5] scsi: bnx2fc: convert bnx2fc_l2_rcv_thread() to workqueue

[Sebastian Andrzej Siewior]

This is not driven by the hotplug conversation but while I am at it
looks like a good candidate. Converting the thread to a workqueue user
removes also the kthread member from struct fcoe_percpu_s.

This driver uses the struct fcoe_percpu_s but it does not need the
crc_eof_page member, only the work item and fcoe_rx_list. So it is
removed there.

We had one thread so we only use the workqueue on the current CPU. If
someone knows how spread this nicely, it would only require the usage of
schedule_work_on() instead schedule_work() :)

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Cc: fcoe-de...@open-fcoe.org
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 59 ---
 include/scsi/libfcoe.h|  1 -
 2 files changed, 12 insertions(+), 48 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c 
b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index 2f66c2ea093c..ca1b5908114d 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -479,7 +479,7 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct 
net_device *dev,
 
__skb_queue_tail(>fcoe_rx_list, skb);
if (bg->fcoe_rx_list.qlen == 1)
-   wake_up_process(bg->kthread);
+   schedule_work(>work);
 
spin_unlock(>fcoe_rx_list.lock);
 
@@ -489,26 +489,20 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct 
net_device *dev,
return -1;
 }
 
-static int bnx2fc_l2_rcv_thread(void *arg)
+static void bnx2fc_l2_rcv_work(struct work_struct *work_s)
 {
-   struct fcoe_percpu_s *bg = arg;
+   struct fcoe_percpu_s *bg;
struct sk_buff *skb;
 
-   set_user_nice(current, MIN_NICE);
-   set_current_state(TASK_INTERRUPTIBLE);
-   while (!kthread_should_stop()) {
-   schedule();
-   spin_lock_bh(>fcoe_rx_list.lock);
-   while ((skb = __skb_dequeue(>fcoe_rx_list)) != NULL) {
-   spin_unlock_bh(>fcoe_rx_list.lock);
-   bnx2fc_recv_frame(skb);
-   spin_lock_bh(>fcoe_rx_list.lock);
-   }
-   __set_current_state(TASK_INTERRUPTIBLE);
+   bg = container_of(work_s, struct fcoe_percpu_s, work);
+
+   spin_lock_bh(>fcoe_rx_list.lock);
+   while ((skb = __skb_dequeue(>fcoe_rx_list)) != NULL) {
spin_unlock_bh(>fcoe_rx_list.lock);
+   bnx2fc_recv_frame(skb);
+   spin_lock_bh(>fcoe_rx_list.lock);
}
-   __set_current_state(TASK_RUNNING);
-   return 0;
+   spin_unlock_bh(>fcoe_rx_list.lock);
 }
 
 
@@ -2564,8 +2558,6 @@ static int bnx2fc_slave_configure(struct scsi_device 
*sdev)
return 0;
 }
 
-static enum cpuhp_state bnx2fc_online_state;
-
 /**
  * bnx2fc_mod_init - module init entry point
  *
@@ -2575,7 +2567,6 @@ static enum cpuhp_state bnx2fc_online_state;
 static int __init bnx2fc_mod_init(void)
 {
struct fcoe_percpu_s *bg;
-   struct task_struct *l2_thread;
int rc = 0;
unsigned int cpu = 0;
struct bnx2fc_percpu_s *p;
@@ -2608,17 +2599,7 @@ static int __init bnx2fc_mod_init(void)
 
bg = _global;
skb_queue_head_init(>fcoe_rx_list);
-   l2_thread = kthread_create(bnx2fc_l2_rcv_thread,
-  (void *)bg,
-  "bnx2fc_l2_thread");
-   if (IS_ERR(l2_thread)) {
-   rc = PTR_ERR(l2_thread);
-   goto free_wq;
-   }
-   wake_up_process(l2_thread);
-   spin_lock_bh(>fcoe_rx_list.lock);
-   bg->kthread = l2_thread;
-   spin_unlock_bh(>fcoe_rx_list.lock);
+   INIT_WORK(>work, bnx2fc_l2_rcv_work);
 
for_each_possible_cpu(cpu) {
p = _cpu(bnx2fc_percpu, cpu);
@@ -2631,8 +2612,6 @@ static int __init bnx2fc_mod_init(void)
 
return 0;
 
-free_wq:
-   destroy_workqueue(bnx2fc_wq);
 release_bt:
bnx2fc_release_transport();
 detach_ft:
@@ -2645,9 +2624,6 @@ static void __exit bnx2fc_mod_exit(void)
 {
LIST_HEAD(to_be_deleted);
struct bnx2fc_hba *hba, *next;
-   struct fcoe_percpu_s *bg;
-   struct task_struct *l2_thread;
-   struct sk_buff *skb;
unsigned int cpu = 0;
 
/*
@@ -2676,18 +2652,7 @@ static void __exit bnx2fc_mod_exit(void)
}
cnic_unregister_driver(CNIC_ULP_FCOE);
 
-   /* Destroy global thread */
-   bg = _global;
-   spin_lock_bh(>fcoe_rx_list.lock);
-   l2_thread = bg->kthread;
-   bg->kthread = NULL;
-   while ((skb = __skb_dequeue(>fcoe_rx_list)) != NULL)
-   kfree_skb(skb);
-
-   spin_unlock_bh(>fcoe_rx_list.lock);
-
-   if (l2_thread)
-   kthread_stop(l2_thread);
+   flush_work(_global.work)

[PATCH 4/5] scsi: bnx2fc: annoate unlock + release for sparse

[Sebastian Andrzej Siewior]

The caller of bnx2fc_abts_cleanup() holds the tgt->tgt_lock lock and it
is expected to release the lock during wait_for_completion() and acquire
the lock afterwards.

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc_io.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_io.c b/drivers/scsi/bnx2fc/bnx2fc_io.c
index 898461b146cc..3eed2453648c 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_io.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_io.c
@@ -1080,6 +1080,8 @@ int bnx2fc_eh_device_reset(struct scsi_cmnd *sc_cmd)
 }
 
 static int bnx2fc_abts_cleanup(struct bnx2fc_cmd *io_req)
+__releases(tgt->tgt_lock)
+__acquires(tgt->tgt_lock)
 {
struct bnx2fc_rport *tgt = io_req->tgt;
int rc = SUCCESS;
-- 
2.11.0

[PATCH 2/5] scsi: bnx2fc: convert per-CPU thread to workqueue

[Sebastian Andrzej Siewior]

The driver creates its own per-CPU threads which are updated based on CPU
hotplug events. It is also possible to use kworkers and remove some of the
infrastructure get the same job done while saving a few lines of code.

bnx2fc_percpu_io_thread() becomes bnx2fc_percpu_io_work() which is
mostly the same code. The outer loop (kthread_should_stop()) gets
removed and the remaining code is shifted to the left.
In bnx2fc_process_new_cqes() the code checked for ->iothread to
decide if there is an active per-CPU thread. With the kworkers this
is no longer possible nor required. The allocation of a new work item
(via bnx2fc_alloc_work()) does no longer happen with the ->fp_work_lock
lock held. It performs only a memory allocation + initialization which
does not require any kind of serialization. The lock is held while
adding the new member to fps->work_list list.

The remaining part is the removal CPU hotplug notifier since it is taken
care by the workqueue code.

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc.h  |   2 +-
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 144 ++
 drivers/scsi/bnx2fc/bnx2fc_hwi.c  |  22 +++---
 3 files changed, 33 insertions(+), 135 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc.h b/drivers/scsi/bnx2fc/bnx2fc.h
index 4fc8ed5fe067..0279cc8de7a0 100644
--- a/drivers/scsi/bnx2fc/bnx2fc.h
+++ b/drivers/scsi/bnx2fc/bnx2fc.h
@@ -168,7 +168,7 @@ extern struct fcoe_percpu_s bnx2fc_global;
 extern struct workqueue_struct *bnx2fc_wq;
 
 struct bnx2fc_percpu_s {
-   struct task_struct *iothread;
+   struct work_struct work;
struct list_head work_list;
spinlock_t fp_work_lock;
 };
diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c 
b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index 93b5a0012417..329922d51f8a 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -614,39 +614,32 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
 }
 
 /**
- * bnx2fc_percpu_io_thread - thread per cpu for ios
+ * bnx2fc_percpu_io_work - work per cpu for ios
  *
- * @arg:   ptr to bnx2fc_percpu_info structure
+ * @work_s:The work struct
  */
-static int bnx2fc_percpu_io_thread(void *arg)
+static void bnx2fc_percpu_io_work(struct work_struct *work_s)
 {
-   struct bnx2fc_percpu_s *p = arg;
+   struct bnx2fc_percpu_s *p;
struct bnx2fc_work *work, *tmp;
LIST_HEAD(work_list);
 
-   set_user_nice(current, MIN_NICE);
-   set_current_state(TASK_INTERRUPTIBLE);
-   while (!kthread_should_stop()) {
-   schedule();
-   spin_lock_bh(>fp_work_lock);
-   while (!list_empty(>work_list)) {
-   list_splice_init(>work_list, _list);
-   spin_unlock_bh(>fp_work_lock);
+   p = container_of(work_s, struct bnx2fc_percpu_s, work);
 
-   list_for_each_entry_safe(work, tmp, _list, list) {
-   list_del_init(>list);
-   bnx2fc_process_cq_compl(work->tgt, work->wqe);
-   kfree(work);
-   }
-
-   spin_lock_bh(>fp_work_lock);
-   }
-   __set_current_state(TASK_INTERRUPTIBLE);
+   spin_lock_bh(>fp_work_lock);
+   while (!list_empty(>work_list)) {
+   list_splice_init(>work_list, _list);
spin_unlock_bh(>fp_work_lock);
-   }
-   __set_current_state(TASK_RUNNING);
 
-   return 0;
+   list_for_each_entry_safe(work, tmp, _list, list) {
+   list_del_init(>list);
+   bnx2fc_process_cq_compl(work->tgt, work->wqe);
+   kfree(work);
+   }
+
+   spin_lock_bh(>fp_work_lock);
+   }
+   spin_unlock_bh(>fp_work_lock);
 }
 
 static struct fc_host_statistics *bnx2fc_get_host_stats(struct Scsi_Host 
*shost)
@@ -2563,73 +2556,6 @@ static struct fcoe_transport bnx2fc_transport = {
.disable = bnx2fc_disable,
 };
 
-/**
- * bnx2fc_percpu_thread_create - Create a receive thread for an
- *  online CPU
- *
- * @cpu: cpu index for the online cpu
- */
-static void bnx2fc_percpu_thread_create(unsigned int cpu)
-{
-   struct bnx2fc_percpu_s *p;
-   struct task_struct *thread;
-
-   p = _cpu(bnx2fc_percpu, cpu);
-
-   thread = kthread_create_on_node(bnx2fc_percpu_io_thread,
-   (void *)p, cpu_to_node(cpu),
-   "bnx2fc_thread/%d", cpu);
-   /* bind thread to the cpu */
-   if (likely(!IS_ERR(thread))) {
-   kthread_bind(thread, cpu);
-   p->io

[REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

[Sebastian Andrzej Siewior]

This is a repost to get the patches applied against v4.11-rc6. mkp's scsi
for-next tree can be merged with no conflicts.

The last repost [0] was not merged and stalled after Martin pinged Chad
[1]. He didn't even reply after tglx pinged him approx two weeks later.

Johannes Thumshirn was so kind to test the FCoE part of the old series
[2]. I did not add a tested-by tag due to the rebase.

If my memory is correct then my first attempt on this (with hotplug
threads back then) was around December 2015.

The whole series is also available at
 git://git.kernel.org/pub/scm/linux/kernel/git/bigeasy/hotplug-staging.git 
scsi/bnx2_v2

[0] http://www.spinics.net/lists/linux-scsi/msg102143.html
[1] http://www.spinics.net/lists/linux-scsi/msg102295.html
[2] http://www.spinics.net/lists/linux-scsi/msg102716.html

Sebastian

[PATCH 1/5] scsi: bnx2i: convert to workqueue

[Sebastian Andrzej Siewior]

The driver creates its own per-CPU threads which are updated based on CPU
hotplug events. It is also possible to use kworkers and remove some of the
infrastructure get the same job done while saving a few lines of code.

The DECLARE_PER_CPU() definition is moved into the header file where it
belongs. bnx2i_percpu_io_thread() becomes bnx2i_percpu_io_work() which is
mostly the same code. The outer loop (kthread_should_stop()) gets removed and
the remaining code is shifted to the left.
bnx2i_queue_scsi_cmd_resp() is mostly the same. The code checked ->iothread to
decide if there is an active per-CPU thread. With the kworkers this is no
longer possible nor required.
The allocation of struct bnx2i_work does not happen with ->p_work_lock held
which is not required. I am unsure about the call-stack so I can't say
if this qualifies it for the allocation with GFP_KERNEL instead of
GFP_ATOMIC (it is not _bh lock but as I said, I don't know the context).
The allocation case has been reversed so the inner if case is called on
!bnx2i_work and is just the invocation one function since the lock is not
held during allocation. The init of the new bnx2i_work struct is now
done also without the ->p_work_lock held: it is a new object, nobody
knows about it yet. It should be enough to hold the lock while adding
this item to the list. I am unsure about that atomic_inc() so I keep
things as they were.

The remaining part is the removal CPU hotplug notifier since it is taken
care by the workqueue code.

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2i/bnx2i.h  |  11 ++---
 drivers/scsi/bnx2i/bnx2i_hwi.c  | 101 +-
 drivers/scsi/bnx2i/bnx2i_init.c | 104 +++-
 3 files changed, 53 insertions(+), 163 deletions(-)

diff --git a/drivers/scsi/bnx2i/bnx2i.h b/drivers/scsi/bnx2i/bnx2i.h
index 89ef1a1678d1..78f67542cbd3 100644
--- a/drivers/scsi/bnx2i/bnx2i.h
+++ b/drivers/scsi/bnx2i/bnx2i.h
@@ -31,7 +31,6 @@
 #include 
 #include 
 #include 
-#include 
 
 #include 
 #include 
@@ -775,12 +774,11 @@ struct bnx2i_work {
 };
 
 struct bnx2i_percpu_s {
-   struct task_struct *iothread;
+   struct work_struct work;
struct list_head work_list;
spinlock_t p_work_lock;
 };
 
-
 /* Global variables */
 extern unsigned int error_mask1, error_mask2;
 extern u64 iscsi_error_mask;
@@ -797,7 +795,7 @@ extern unsigned int rq_size;
 
 extern struct device_attribute *bnx2i_dev_attributes[];
 
-
+DECLARE_PER_CPU(struct bnx2i_percpu_s, bnx2i_percpu);
 
 /*
  * Function Prototypes
@@ -875,8 +873,5 @@ extern void bnx2i_print_active_cmd_queue(struct bnx2i_conn 
*conn);
 extern void bnx2i_print_xmit_pdu_queue(struct bnx2i_conn *conn);
 extern void bnx2i_print_recv_state(struct bnx2i_conn *conn);
 
-extern int bnx2i_percpu_io_thread(void *arg);
-extern int bnx2i_process_scsi_cmd_resp(struct iscsi_session *session,
-  struct bnx2i_conn *bnx2i_conn,
-  struct cqe *cqe);
+extern void bnx2i_percpu_io_work(struct work_struct *work);
 #endif
diff --git a/drivers/scsi/bnx2i/bnx2i_hwi.c b/drivers/scsi/bnx2i/bnx2i_hwi.c
index 42921dbba927..9be58f6523b3 100644
--- a/drivers/scsi/bnx2i/bnx2i_hwi.c
+++ b/drivers/scsi/bnx2i/bnx2i_hwi.c
@@ -19,8 +19,6 @@
 #include 
 #include "bnx2i.h"
 
-DECLARE_PER_CPU(struct bnx2i_percpu_s, bnx2i_percpu);
-
 /**
  * bnx2i_get_cid_num - get cid from ep
  * @ep:endpoint pointer
@@ -1350,9 +1348,9 @@ int bnx2i_send_fw_iscsi_init_msg(struct bnx2i_hba *hba)
  *
  * process SCSI CMD Response CQE & complete the request to SCSI-ML
  */
-int bnx2i_process_scsi_cmd_resp(struct iscsi_session *session,
-   struct bnx2i_conn *bnx2i_conn,
-   struct cqe *cqe)
+static int bnx2i_process_scsi_cmd_resp(struct iscsi_session *session,
+  struct bnx2i_conn *bnx2i_conn,
+  struct cqe *cqe)
 {
struct iscsi_conn *conn = bnx2i_conn->cls_conn->dd_data;
struct bnx2i_hba *hba = bnx2i_conn->hba;
@@ -1862,45 +1860,37 @@ static void bnx2i_process_cmd_cleanup_resp(struct 
iscsi_session *session,
 
 
 /**
- * bnx2i_percpu_io_thread - thread per cpu for ios
+ * bnx2i_percpu_io_work - thread per cpu for ios
  *
- * @arg:   ptr to bnx2i_percpu_info structure
+ * @work_s:The work struct
  */
-int bnx2i_percpu_io_thread(void *arg)
+void bnx2i_percpu_io_work(struct work_struct *work_s)
 {
-   struct bnx2i_percpu_s *p = arg;
+   struct bnx2i_percpu_s *p;
struct bnx2i_work *work, *tmp;
LIST_HEAD(work_list);
 
-   set_user_nice(current, MIN_NICE);
+   p = container_of(work_s, struct bnx2i_percpu_s, 

Re: [lkp-robot] [rcu] b332151a29: kernel_BUG_at_mm/slab.c

[Sebastian Andrzej Siewior]

On 2017-01-20 08:32:37 [-0800], Jens Axboe wrote:
> That's alright, sounds like it's not a -next regression, but rather something
> that is already broken. I can reproduce a lot of breakage if I enable
> CONFIG_DEBUG_TEST_DRIVER_REMOVE, in fact my system doesn't boot at all. This
> is the first bug:
> 
> [   18.247895] [ cut here ]
> [   18.247907] WARNING: CPU: 21 PID: 2223 at drivers/ata/libata-core.c:6522 
> ata_host_detach+0x11b]
> [   18.247908] Modules linked in: igb(+) ahci(+) libahci i2c_algo_bit dca 
> libata nvme(+) nvme_core
> [   18.247917] CPU: 21 PID: 2223 Comm: systemd-udevd Tainted: GW  
>  4.10.0-rc4+ #30
> [   18.247919] Hardware name: Dell Inc. PowerEdge T630/0NT78X, BIOS 2.3.4 
> 11/09/2016
> [   18.247919] Call Trace:
> [   18.247928]  dump_stack+0x68/0x93
> [   18.247934]  __warn+0xc6/0xe0
> [   18.247937]  warn_slowpath_null+0x18/0x20
> [   18.247943]  ata_host_detach+0x11b/0x120 [libata]
…

> and it's even more downhill from there. That option is marked unstable, are we
> expecting it to work right now?

Well, as per 248ff0216543 ("driver core: Make Kconfig text for
DEBUG_TEST_DRIVER_REMOVE stronger"):

|   The current state of driver removal is not great.
|   CONFIG_DEBUG_TEST_DRIVER_REMOVE finds lots of errors. The help text
|   currently undersells exactly how many errors this option will find. Add
|   a bit more description to indicate this option shouldn't be turned on
|   unless you actually want to debug driver removal. The text can be
|   changed later when more drivers are fixed up.

so it looks like the option is working but it uncovers bugs. I've put
you in TO because the breakage in kvm test went away after I disabled
the MQ support in SCSI. So I *assumed* that MQ was not doing something
right in the removal path. I don't know if this libata-core backtrace is
a false positive or not.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [lkp-robot] [rcu] b332151a29: kernel_BUG_at_mm/slab.c

[Sebastian Andrzej Siewior]

On 2017-01-20 08:09:36 [-0800], Jens Axboe wrote:
> Is there a full trace of this?

[3.654003] scsi host0: scsi_debug: version 1.86 [20160430]
[3.654003]   dev_size_mb=8, opts=0x0, submit_queues=1, statistics=0
[3.660755] scsi 0:0:0:0: Direct-Access Linuxscsi_debug   0186 
PQ: 0 ANSI: 7
[3.711231] sd 0:0:0:0: [sda] 16384 512-byte logical blocks: (8.39 MB/8.00 
MiB)
[3.716202] sd 0:0:0:0: [sda] Write Protect is off
[3.717244] sd 0:0:0:0: [sda] Mode Sense: 73 00 10 08
[3.725059] sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, 
supports DPO and FUA
[3.795093] sd 0:0:0:0: [sda] Attached SCSI disk
[3.796686] sd 0:0:0:0: [sda] Synchronizing SCSI cache
[3.804770] sd 0:0:0:0: [sda] 16384 512-byte logical blocks: (8.39 MB/8.00 
MiB)
[3.809748] sd 0:0:0:0: [sda] Write Protect is off
[3.810806] sd 0:0:0:0: [sda] Mode Sense: 73 00 10 08
[3.818599] sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, 
supports DPO and FUA
[3.830894] kobject (be01a5fc): tried to init an initialized object, 
something is seriously wrong.
[3.832820] CPU: 6 PID: 6 Comm: kworker/u14:0 Not tainted 4.9.0 #86
[3.834172] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 
1.10.1-1 04/01/2014
[3.835886] Workqueue: events_unbound async_run_entry_fn
[3.837028]  80079da8 83a5f6cc be01a5fc be01a5fc 80079dc4 83a61e33 842a883c 
be01a5fc
[3.838802]   84397488 be01a108 80079dec 83a46afa be01a5d8 840670a0 
be01a5d8
[3.840570]  be01a108 bd983468 be01a108 bd983470 be01a5d8 80079e14 83a3c857 
be01a5d8
[3.842350] Call Trace:
[3.842884]  [<83a5f6cc>] dump_stack+0x58/0x7c
[3.843834]  [<83a61e33>] kobject_init+0x73/0x80
[3.844828]  [<83a46afa>] blk_mq_register_dev+0x2a/0x110
[3.845971]  [<83a3c857>] blk_register_queue+0x87/0x140
[3.847085]  [<83a49c7e>] device_add_disk+0x1ce/0x470
[3.848170]  [<83c3d328>] sd_probe_async+0xe8/0x1b0
[3.849207]  [<83672347>] async_run_entry_fn+0x37/0xe0
[3.850313]  [<836694f7>] process_one_work+0x1b7/0x3e0
[3.851414]  [<8366947c>] ? process_one_work+0x13c/0x3e0
[3.852552]  [<83669759>] worker_thread+0x39/0x460
[3.853569]  [<83669720>] ? process_one_work+0x3e0/0x3e0
[3.854717]  [<8366f394>] kthread+0xb4/0xd0
[3.855611]  [<83f8cb4d>] ? _raw_spin_unlock_irq+0x2d/0x50
[3.856775]  [<8366f2e0>] ? __kthread_create_on_node+0x160/0x160
[3.858066]  [<8366f2e0>] ? __kthread_create_on_node+0x160/0x160
[3.859340]  [<83f8d363>] ret_from_fork+0x1b/0x28
[3.863951] kobject (ff0ca36c): tried to init an initialized object, 
something is seriously wrong.
[3.865875] CPU: 6 PID: 6 Comm: kworker/u14:0 Not tainted 4.9.0 #86
[3.867202] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 
1.10.1-1 04/01/2014
[3.868924] Workqueue: events_unbound async_run_entry_fn
[3.870079]  80079da8 83a5f6cc ff0ca36c be01a5fc 80079dc4 83a61e33 842a883c 
ff0ca36c
[3.871846]  80079dc4 84397474 be01a108 80079dec 83a46b1c be01a5d8 840670a0 
be01a5d8
[3.873605]  be01a108 bd983468 be01a108 bd983470 be01a5d8 80079e14 83a3c857 
be01a5d8
[3.875395] Call Trace:
[3.875928]  [<83a5f6cc>] dump_stack+0x58/0x7c
[3.876878]  [<83a61e33>] kobject_init+0x73/0x80
[3.877884]  [<83a46b1c>] blk_mq_register_dev+0x4c/0x110
[3.879018]  [<83a3c857>] blk_register_queue+0x87/0x140
[3.880136]  [<83a49c7e>] device_add_disk+0x1ce/0x470
[3.881227]  [<83c3d328>] sd_probe_async+0xe8/0x1b0
[3.882283]  [<83672347>] async_run_entry_fn+0x37/0xe0
[3.883381]  [<836694f7>] process_one_work+0x1b7/0x3e0
[3.884481]  [<8366947c>] ? process_one_work+0x13c/0x3e0
[3.885613]  [<83669759>] worker_thread+0x39/0x460
[3.886658]  [<83669720>] ? process_one_work+0x3e0/0x3e0
[3.887791]  [<8366f394>] kthread+0xb4/0xd0
[3.888680]  [<83f8cb4d>] ? _raw_spin_unlock_irq+0x2d/0x50
[3.889881]  [<8366f2e0>] ? __kthread_create_on_node+0x160/0x160
[3.891189]  [<8366f2e0>] ? __kthread_create_on_node+0x160/0x160
[3.892506]  [<83f8d363>] ret_from_fork+0x1b/0x28
[3.893563] kobject (ff21b36c): tried to init an initialized object, 
something is seriously wrong.
[3.895559] CPU: 6 PID: 6 Comm: kworker/u14:0 Not tainted 4.9.0 #86
[3.896938] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 
1.10.1-1 04/01/2014
[3.898741] Workqueue: events_unbound async_run_entry_fn
[3.899911]  80079da8 83a5f6cc ff21b36c be01a5fc 80079dc4 83a61e33 842a883c 
ff21b36c
[3.901739]  80079dc4 84397474 be01a108 80079dec 83a46b1c be01a5d8 840670a0 
be01a5d8
[3.903558]  be01a108 bd983468 be01a108 bd983470 be01a5d8 80079e14 83a3c857 
be01a5d8
[3.905381] Call Trace:
[3.905936]  [<83a5f6cc>] dump_stack+0x58/0x7c
[3.906913]  [<83a61e33>] kobject_init+0x73/0x80
[3.907931]  [<83a46b1c>] blk_mq_register_dev+0x4c/0x110
[3.909108]  [<83a3c857>] blk_register_queue+0x87/0x140
[3.910267]  [<83a49c7e>] device_add_disk+0x1ce/0x470
[

Re: [lkp-robot] [rcu] b332151a29: kernel_BUG_at_mm/slab.c

[Sebastian Andrzej Siewior]

On 2017-01-19 09:02:16 [+0800], kernel test robot wrote:
> test-description: Trinity is a linux system call fuzz tester.

you don't even get to fire up trinity. With and without the patch you
crash very early.

> +-+++
> | | d5f6ab9c11 | 
> b332151a29 |
> +-+++
> | boot_successes  | 0  | 0
>   |
> | boot_failures   | 6  | 8
>   |
> | WARNING:at_include/linux/kref.h:#kobject_get| 6  | 8
>   |
> | WARNING:at_arch/x86/mm/dump_pagetables.c:#note_page | 2  | 2
>   |
> | kernel_BUG_at_mm/slab.c | 0  | 4
>   |
> | invalid_opcode:#[##]PREEMPT_SMP | 0  | 4
>   |
> | Kernel_panic-not_syncing:Fatal_exception| 0  | 6
>   |
> | BUG:unable_to_handle_kernel | 0  | 2
>   |
> | Oops| 0  | 2
>   |
> +-+++

There is no successful boot. The pattern changes with patch in question
applied.

> [8.044624] sd 0:0:0:0: [sda] Synchronizing SCSI cache
> [8.055721] slab: double free detected in cache 'kmalloc-32', objp 8af558c0
> [8.057138] [ cut here ]
> [8.058085] kernel BUG at mm/slab.c:2624!
> [8.059255] invalid opcode:  [#1] PREEMPT SMP

yes. With and without the patch there is a lot of wrong stuff like
complains about a kobject initialized again. This leads to a double free
at some point.

What happens is the following: CONFIG_SCSI_DEBUG is enabled which adds a
dummy host controller with a dummy disk. This gets probed during boot.
Since you also enabled CONFIG_DEBUG_TEST_DRIVER_REMOVE it gets removed
and re-added. The request_queue in genhd disk is re-used while the disk
is added for the second time:

[1.314404] scsi host0: scsi_debug: version 1.86 [20160430]
[1.314404]   dev_size_mb=8, opts=0x0, submit_queues=1, statistics=0
[1.315994] scsi 0:0:0:0: Direct-Access Linuxscsi_debug   0186 
PQ: 0 ANSI: 7
[1.351052] sd 0:0:0:0: [sda] 16384 512-byte logical blocks: (8.39 MB/8.00 
MiB)
[1.355916] sd 0:0:0:0: [sda] Write Protect is off
[1.356838] sd 0:0:0:0: [sda] Mode Sense: 73 00 10 08
[1.364455] sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, 
supports DPO and FUA
[1.437642] sd 0:0:0:0: [sda] Attached SCSI disk
[1.438413] sd 0:0:0:0: [sda] Synchronizing SCSI cache
[1.445868] sd 0:0:0:0: [sda] 16384 512-byte logical blocks: (8.39 MB/8.00 
MiB)
[1.450819] sd 0:0:0:0: [sda] Write Protect is off
[1.451853] sd 0:0:0:0: [sda] Mode Sense: 73 00 10 08
[1.459636] sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, 
supports DPO and FUA
[1.471446] kobject (beb87d44): tried to init an initialized object, 
something is seriously wrong.

Since you also need CONFIG_SCSI_MQ_DEFAULT enabled I assume the MQ block
code is buggy here.
But commit b332151a29 in Paul's tree innocent.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] scsi: bnx2fc: clean up header definitions

[Sebastian Andrzej Siewior]

- All symbols which are only used within one .c file are marked static
  and removed from the bnx2fc.h file if possible.

- the declarion of bnx2fc_percpu is moved into the header file

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc.h  |  5 +
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 19 +--
 drivers/scsi/bnx2fc/bnx2fc_hwi.c  |  6 ++
 3 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc.h b/drivers/scsi/bnx2fc/bnx2fc.h
index fdd89935cac7..64308584405d 100644
--- a/drivers/scsi/bnx2fc/bnx2fc.h
+++ b/drivers/scsi/bnx2fc/bnx2fc.h
@@ -172,6 +172,7 @@ struct bnx2fc_percpu_s {
struct list_head work_list;
spinlock_t fp_work_lock;
 };
+DECLARE_PER_CPU(struct bnx2fc_percpu_s, bnx2fc_percpu);
 
 struct bnx2fc_fw_stats {
u64 fc_crc_cnt;
@@ -513,7 +514,6 @@ void bnx2fc_cmd_mgr_free(struct bnx2fc_cmd_mgr *cmgr);
 void bnx2fc_get_link_state(struct bnx2fc_hba *hba);
 char *bnx2fc_get_next_rqe(struct bnx2fc_rport *tgt, u8 num_items);
 void bnx2fc_return_rqe(struct bnx2fc_rport *tgt, u8 num_items);
-int bnx2fc_get_paged_crc_eof(struct sk_buff *skb, int tlen);
 int bnx2fc_send_rrq(struct bnx2fc_cmd *aborted_io_req);
 int bnx2fc_send_adisc(struct bnx2fc_rport *tgt, struct fc_frame *fp);
 int bnx2fc_send_logo(struct bnx2fc_rport *tgt, struct fc_frame *fp);
@@ -537,7 +537,6 @@ void bnx2fc_init_task(struct bnx2fc_cmd *io_req,
 void bnx2fc_add_2_sq(struct bnx2fc_rport *tgt, u16 xid);
 void bnx2fc_ring_doorbell(struct bnx2fc_rport *tgt);
 int bnx2fc_eh_abort(struct scsi_cmnd *sc_cmd);
-int bnx2fc_eh_host_reset(struct scsi_cmnd *sc_cmd);
 int bnx2fc_eh_target_reset(struct scsi_cmnd *sc_cmd);
 int bnx2fc_eh_device_reset(struct scsi_cmnd *sc_cmd);
 void bnx2fc_rport_event_handler(struct fc_lport *lport,
@@ -570,8 +569,6 @@ struct fc_seq *bnx2fc_elsct_send(struct fc_lport *lport, 
u32 did,
   struct fc_frame *,
   void *),
  void *arg, u32 timeout);
-void bnx2fc_arm_cq(struct bnx2fc_rport *tgt);
-int bnx2fc_process_new_cqes(struct bnx2fc_rport *tgt);
 void bnx2fc_process_cq_compl(struct bnx2fc_rport *tgt, u16 wqe);
 struct bnx2fc_rport *bnx2fc_tgt_lookup(struct fcoe_port *port,
 u32 port_id);
diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c 
b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index 79ff0ed2b364..97424e0eaa23 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -49,7 +49,7 @@ struct workqueue_struct *bnx2fc_wq;
  * Here the io threads are per cpu but the l2 thread is just one
  */
 struct fcoe_percpu_s bnx2fc_global;
-DEFINE_SPINLOCK(bnx2fc_global_lock);
+static DEFINE_SPINLOCK(bnx2fc_global_lock);
 
 static struct cnic_ulp_ops bnx2fc_cnic_cb;
 static struct libfc_function_template bnx2fc_libfc_fcn_templ;
@@ -107,22 +107,22 @@ MODULE_PARM_DESC(debug_logging,
"\t\t0x10 - fcoe L2 fame related logs.\n"
"\t\t0xff - LOG all messages.");
 
-uint bnx2fc_devloss_tmo;
+static uint bnx2fc_devloss_tmo;
 module_param_named(devloss_tmo, bnx2fc_devloss_tmo, uint, S_IRUGO);
 MODULE_PARM_DESC(devloss_tmo, " Change devloss_tmo for the remote ports "
"attached via bnx2fc.");
 
-uint bnx2fc_max_luns = BNX2FC_MAX_LUN;
+static uint bnx2fc_max_luns = BNX2FC_MAX_LUN;
 module_param_named(max_luns, bnx2fc_max_luns, uint, S_IRUGO);
 MODULE_PARM_DESC(max_luns, " Change the default max_lun per SCSI host. Default 
"
"0x.");
 
-uint bnx2fc_queue_depth;
+static uint bnx2fc_queue_depth;
 module_param_named(queue_depth, bnx2fc_queue_depth, uint, S_IRUGO);
 MODULE_PARM_DESC(queue_depth, " Change the default queue depth of SCSI devices 
"
"attached via bnx2fc.");
 
-uint bnx2fc_log_fka;
+static uint bnx2fc_log_fka;
 module_param_named(log_fka, bnx2fc_log_fka, uint, S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(log_fka, " Print message to kernel log when fcoe is "
"initiating a FIP keep alive when debug logging is enabled.");
@@ -167,7 +167,7 @@ static void bnx2fc_clean_rx_queue(struct fc_lport *lp)
spin_unlock_bh(>fcoe_rx_list.lock);
 }
 
-int bnx2fc_get_paged_crc_eof(struct sk_buff *skb, int tlen)
+static int bnx2fc_get_paged_crc_eof(struct sk_buff *skb, int tlen)
 {
int rc;
spin_lock(_global_lock);
@@ -1396,10 +1396,9 @@ static struct bnx2fc_hba *bnx2fc_hba_create(struct 
cnic_dev *cnic)
return NULL;
 }
 
-static struct bnx2fc_interface *
-bnx2fc_interface_create(struct bnx2fc_hba *hba,
-   struct net_device *netdev,
-   enum fip_state fip_mode)
+static st

[PATCH 5/5] scsi: bnx2fc: convert bnx2fc_l2_rcv_thread() to workqueue

[Sebastian Andrzej Siewior]

This is not driven by the hotplug conversation but while I am at it
looks like a good candidate. Converting the thread to a workqueue user
removes also the kthread member from struct fcoe_percpu_s.

This driver uses the struct fcoe_percpu_s but it does not need the
crc_eof_page member, only the work item and fcoe_rx_list. So it is
removed there.

We had one thread so we only use the workqueue on the current CPU. If
someone knows how spread this nicely, it would only require the usage of
schedule_work_on() instead schedule_work() :)

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Cc: fcoe-de...@open-fcoe.org
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 57 +--
 include/scsi/libfcoe.h|  1 -
 2 files changed, 12 insertions(+), 46 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c 
b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index 97424e0eaa23..698348b9fa99 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -479,7 +479,7 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct 
net_device *dev,
 
__skb_queue_tail(>fcoe_rx_list, skb);
if (bg->fcoe_rx_list.qlen == 1)
-   wake_up_process(bg->kthread);
+   schedule_work(>work);
 
spin_unlock(>fcoe_rx_list.lock);
 
@@ -489,26 +489,20 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct 
net_device *dev,
return -1;
 }
 
-static int bnx2fc_l2_rcv_thread(void *arg)
+static void bnx2fc_l2_rcv_work(struct work_struct *work_s)
 {
-   struct fcoe_percpu_s *bg = arg;
+   struct fcoe_percpu_s *bg;
struct sk_buff *skb;
 
-   set_user_nice(current, MIN_NICE);
-   set_current_state(TASK_INTERRUPTIBLE);
-   while (!kthread_should_stop()) {
-   schedule();
-   spin_lock_bh(>fcoe_rx_list.lock);
-   while ((skb = __skb_dequeue(>fcoe_rx_list)) != NULL) {
-   spin_unlock_bh(>fcoe_rx_list.lock);
-   bnx2fc_recv_frame(skb);
-   spin_lock_bh(>fcoe_rx_list.lock);
-   }
-   __set_current_state(TASK_INTERRUPTIBLE);
+   bg = container_of(work_s, struct fcoe_percpu_s, work);
+
+   spin_lock_bh(>fcoe_rx_list.lock);
+   while ((skb = __skb_dequeue(>fcoe_rx_list)) != NULL) {
spin_unlock_bh(>fcoe_rx_list.lock);
+   bnx2fc_recv_frame(skb);
+   spin_lock_bh(>fcoe_rx_list.lock);
}
-   __set_current_state(TASK_RUNNING);
-   return 0;
+   spin_unlock_bh(>fcoe_rx_list.lock);
 }
 
 
@@ -2574,7 +2568,6 @@ static int bnx2fc_slave_configure(struct scsi_device 
*sdev)
 static int __init bnx2fc_mod_init(void)
 {
struct fcoe_percpu_s *bg;
-   struct task_struct *l2_thread;
int rc = 0;
unsigned int cpu = 0;
struct bnx2fc_percpu_s *p;
@@ -2607,17 +2600,7 @@ static int __init bnx2fc_mod_init(void)
 
bg = _global;
skb_queue_head_init(>fcoe_rx_list);
-   l2_thread = kthread_create(bnx2fc_l2_rcv_thread,
-  (void *)bg,
-  "bnx2fc_l2_thread");
-   if (IS_ERR(l2_thread)) {
-   rc = PTR_ERR(l2_thread);
-   goto free_wq;
-   }
-   wake_up_process(l2_thread);
-   spin_lock_bh(>fcoe_rx_list.lock);
-   bg->kthread = l2_thread;
-   spin_unlock_bh(>fcoe_rx_list.lock);
+   INIT_WORK(>work, bnx2fc_l2_rcv_work);
 
for_each_possible_cpu(cpu) {
p = _cpu(bnx2fc_percpu, cpu);
@@ -2630,8 +2613,6 @@ static int __init bnx2fc_mod_init(void)
 
return 0;
 
-free_wq:
-   destroy_workqueue(bnx2fc_wq);
 release_bt:
bnx2fc_release_transport();
 detach_ft:
@@ -2644,9 +2625,6 @@ static void __exit bnx2fc_mod_exit(void)
 {
LIST_HEAD(to_be_deleted);
struct bnx2fc_hba *hba, *next;
-   struct fcoe_percpu_s *bg;
-   struct task_struct *l2_thread;
-   struct sk_buff *skb;
unsigned int cpu = 0;
 
/*
@@ -2675,18 +2653,7 @@ static void __exit bnx2fc_mod_exit(void)
}
cnic_unregister_driver(CNIC_ULP_FCOE);
 
-   /* Destroy global thread */
-   bg = _global;
-   spin_lock_bh(>fcoe_rx_list.lock);
-   l2_thread = bg->kthread;
-   bg->kthread = NULL;
-   while ((skb = __skb_dequeue(>fcoe_rx_list)) != NULL)
-   kfree_skb(skb);
-
-   spin_unlock_bh(>fcoe_rx_list.lock);
-
-   if (l2_thread)
-   kthread_stop(l2_thread);
+   flush_work(_global.work);
 
for_each_possible_cpu(cpu) {
struct bnx2fc_percpu_s *p;
diff --git a/include/scsi/libfcoe.h b/include/scsi/libfcoe.h
index 722d3264d3bf..ca6fb3c0f172 100644
--- a

[PATCH 4/5] scsi: bnx2fc: annoate unlock + release for sparse

[Sebastian Andrzej Siewior]

The caller of bnx2fc_abts_cleanup() holds the tgt->tgt_lock lock and it
is expected to release the lock during wait_for_completion() and acquire
the lock afterwards.

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc_io.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_io.c b/drivers/scsi/bnx2fc/bnx2fc_io.c
index f501095f91ac..634e1e539850 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_io.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_io.c
@@ -1080,6 +1080,8 @@ int bnx2fc_eh_device_reset(struct scsi_cmnd *sc_cmd)
 }
 
 static int bnx2fc_abts_cleanup(struct bnx2fc_cmd *io_req)
+__releases(tgt->tgt_lock)
+__acquires(tgt->tgt_lock)
 {
struct bnx2fc_rport *tgt = io_req->tgt;
int rc = SUCCESS;
-- 
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

bnx2i + bnx2fc: convert to generic workqueue (#2)

[Sebastian Andrzej Siewior]

This is the a repost + fixups to get the patches applied against v4.9-rc4.
mkp's scsi for-next tree can be merged with no conflicts.

The last repost was not merged and stalled after Chad Dupuis said to hold on
because he is testing but never came back with the results [0]. Johannes
confirmed that he did some testing of bnx2fc but he is not sure about bnx2i
[1]. hch asked later about something different but wanted them in since they
were tested as-is [2]. And now I renamed kworker to workqueue as requested by
hch [3].

The whole series is also available at
 git://git.kernel.org/pub/scm/linux/kernel/git/bigeasy/hotplug-staging.git 
scsi/bnx2

[0] https://lkml.kernel.org/r/yq137l2mm13@sermon.lab.mkp.net
[1] https://lkml.kernel.org/r/20161108072434.gnnhmrkkmxroy...@linux-x5ow.site
[2] https://lkml.kernel.org/r/20161107191025.ga5...@lst.de
[3] https://lkml.kernel.org/r/20161118145504.ga9...@lst.de

Sebastian

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] scsi: bnx2i: convert to workqueue

[Sebastian Andrzej Siewior]

The driver creates its own per-CPU threads which are updated based on CPU
hotplug events. It is also possible to use kworkers and remove some of the
infrastructure get the same job done while saving a few lines of code.

The DECLARE_PER_CPU() definition is moved into the header file where it
belongs. bnx2i_percpu_io_thread() becomes bnx2i_percpu_io_work() which is
mostly the same code. The outer loop (kthread_should_stop()) gets removed and
the remaining code is shifted to the left.
bnx2i_queue_scsi_cmd_resp() is mostly the same. The code checked ->iothread to
decide if there is an active per-CPU thread. With the kworkers this is no
longer possible nor required.
The allocation of struct bnx2i_work does not happen with ->p_work_lock held
which is not required. I am unsure about the call-stack so I can't say
if this qualifies it for the allocation with GFP_KERNEL instead of
GFP_ATOMIC (it is not _bh lock but as I said, I don't know the context).
The allocation case has been reversed so the inner if case is called on
!bnx2i_work and is just the invocation one function since the lock is not
held during allocation. The init of the new bnx2i_work struct is now
done also without the ->p_work_lock held: it is a new object, nobody
knows about it yet. It should be enough to hold the lock while adding
this item to the list. I am unsure about that atomic_inc() so I keep
things as they were.

The remaining part is the removal CPU hotplug notifier since it is taken
care by the workqueue code.

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2i/bnx2i.h  |  11 +---
 drivers/scsi/bnx2i/bnx2i_hwi.c  | 101 ++---
 drivers/scsi/bnx2i/bnx2i_init.c | 121 +++-
 3 files changed, 53 insertions(+), 180 deletions(-)

diff --git a/drivers/scsi/bnx2i/bnx2i.h b/drivers/scsi/bnx2i/bnx2i.h
index ed7f3228e234..78cdc493bab5 100644
--- a/drivers/scsi/bnx2i/bnx2i.h
+++ b/drivers/scsi/bnx2i/bnx2i.h
@@ -31,7 +31,6 @@
 #include 
 #include 
 #include 
-#include 
 
 #include 
 #include 
@@ -775,12 +774,11 @@ struct bnx2i_work {
 };
 
 struct bnx2i_percpu_s {
-   struct task_struct *iothread;
+   struct work_struct work;
struct list_head work_list;
spinlock_t p_work_lock;
 };
 
-
 /* Global variables */
 extern unsigned int error_mask1, error_mask2;
 extern u64 iscsi_error_mask;
@@ -797,7 +795,7 @@ extern unsigned int rq_size;
 
 extern struct device_attribute *bnx2i_dev_attributes[];
 
-
+DECLARE_PER_CPU(struct bnx2i_percpu_s, bnx2i_percpu);
 
 /*
  * Function Prototypes
@@ -875,8 +873,5 @@ extern void bnx2i_print_active_cmd_queue(struct bnx2i_conn 
*conn);
 extern void bnx2i_print_xmit_pdu_queue(struct bnx2i_conn *conn);
 extern void bnx2i_print_recv_state(struct bnx2i_conn *conn);
 
-extern int bnx2i_percpu_io_thread(void *arg);
-extern int bnx2i_process_scsi_cmd_resp(struct iscsi_session *session,
-  struct bnx2i_conn *bnx2i_conn,
-  struct cqe *cqe);
+extern void bnx2i_percpu_io_work(struct work_struct *work);
 #endif
diff --git a/drivers/scsi/bnx2i/bnx2i_hwi.c b/drivers/scsi/bnx2i/bnx2i_hwi.c
index 42921dbba927..9be58f6523b3 100644
--- a/drivers/scsi/bnx2i/bnx2i_hwi.c
+++ b/drivers/scsi/bnx2i/bnx2i_hwi.c
@@ -19,8 +19,6 @@
 #include 
 #include "bnx2i.h"
 
-DECLARE_PER_CPU(struct bnx2i_percpu_s, bnx2i_percpu);
-
 /**
  * bnx2i_get_cid_num - get cid from ep
  * @ep:endpoint pointer
@@ -1350,9 +1348,9 @@ int bnx2i_send_fw_iscsi_init_msg(struct bnx2i_hba *hba)
  *
  * process SCSI CMD Response CQE & complete the request to SCSI-ML
  */
-int bnx2i_process_scsi_cmd_resp(struct iscsi_session *session,
-   struct bnx2i_conn *bnx2i_conn,
-   struct cqe *cqe)
+static int bnx2i_process_scsi_cmd_resp(struct iscsi_session *session,
+  struct bnx2i_conn *bnx2i_conn,
+  struct cqe *cqe)
 {
struct iscsi_conn *conn = bnx2i_conn->cls_conn->dd_data;
struct bnx2i_hba *hba = bnx2i_conn->hba;
@@ -1862,45 +1860,37 @@ static void bnx2i_process_cmd_cleanup_resp(struct 
iscsi_session *session,
 
 
 /**
- * bnx2i_percpu_io_thread - thread per cpu for ios
+ * bnx2i_percpu_io_work - thread per cpu for ios
  *
- * @arg:   ptr to bnx2i_percpu_info structure
+ * @work_s:The work struct
  */
-int bnx2i_percpu_io_thread(void *arg)
+void bnx2i_percpu_io_work(struct work_struct *work_s)
 {
-   struct bnx2i_percpu_s *p = arg;
+   struct bnx2i_percpu_s *p;
struct bnx2i_work *work, *tmp;
LIST_HEAD(work_list);
 
-   set_user_nice(current, MIN_NICE);
+   p = container_of(work_s, struct bnx2i_percpu_s, 

[PATCH 2/5] scsi: bnx2fc: convert per-CPU thread to workqueue

[Sebastian Andrzej Siewior]

The driver creates its own per-CPU threads which are updated based on CPU
hotplug events. It is also possible to use kworkers and remove some of the
infrastructure get the same job done while saving a few lines of code.

bnx2fc_percpu_io_thread() becomes bnx2fc_percpu_io_work() which is
mostly the same code. The outer loop (kthread_should_stop()) gets
removed and the remaining code is shifted to the left.
In bnx2fc_process_new_cqes() the code checked for ->iothread to
decide if there is an active per-CPU thread. With the kworkers this
is no longer possible nor required. The allocation of a new work item
(via bnx2fc_alloc_work()) does no longer happen with the ->fp_work_lock
lock held. It performs only a memory allocation + initialization which
does not require any kind of serialization. The lock is held while
adding the new member to fps->work_list list.

The remaining part is the removal CPU hotplug notifier since it is taken
care by the workqueue code.

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc.h  |   2 +-
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 157 ++
 drivers/scsi/bnx2fc/bnx2fc_hwi.c  |  22 +++---
 3 files changed, 32 insertions(+), 149 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc.h b/drivers/scsi/bnx2fc/bnx2fc.h
index fdd4eb4e41b2..fdd89935cac7 100644
--- a/drivers/scsi/bnx2fc/bnx2fc.h
+++ b/drivers/scsi/bnx2fc/bnx2fc.h
@@ -168,7 +168,7 @@ extern struct fcoe_percpu_s bnx2fc_global;
 extern struct workqueue_struct *bnx2fc_wq;
 
 struct bnx2fc_percpu_s {
-   struct task_struct *iothread;
+   struct work_struct work;
struct list_head work_list;
spinlock_t fp_work_lock;
 };
diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c 
b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index f9ddb6156f14..79ff0ed2b364 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -127,13 +127,6 @@ module_param_named(log_fka, bnx2fc_log_fka, uint, 
S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(log_fka, " Print message to kernel log when fcoe is "
"initiating a FIP keep alive when debug logging is enabled.");
 
-static int bnx2fc_cpu_callback(struct notifier_block *nfb,
-unsigned long action, void *hcpu);
-/* notification function for CPU hotplug events */
-static struct notifier_block bnx2fc_cpu_notifier = {
-   .notifier_call = bnx2fc_cpu_callback,
-};
-
 static inline struct net_device *bnx2fc_netdev(const struct fc_lport *lport)
 {
return ((struct bnx2fc_interface *)
@@ -621,39 +614,32 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
 }
 
 /**
- * bnx2fc_percpu_io_thread - thread per cpu for ios
+ * bnx2fc_percpu_io_work - work per cpu for ios
  *
- * @arg:   ptr to bnx2fc_percpu_info structure
+ * @work_s:The work struct
  */
-static int bnx2fc_percpu_io_thread(void *arg)
+static void bnx2fc_percpu_io_work(struct work_struct *work_s)
 {
-   struct bnx2fc_percpu_s *p = arg;
+   struct bnx2fc_percpu_s *p;
struct bnx2fc_work *work, *tmp;
LIST_HEAD(work_list);
 
-   set_user_nice(current, MIN_NICE);
-   set_current_state(TASK_INTERRUPTIBLE);
-   while (!kthread_should_stop()) {
-   schedule();
-   spin_lock_bh(>fp_work_lock);
-   while (!list_empty(>work_list)) {
-   list_splice_init(>work_list, _list);
-   spin_unlock_bh(>fp_work_lock);
+   p = container_of(work_s, struct bnx2fc_percpu_s, work);
 
-   list_for_each_entry_safe(work, tmp, _list, list) {
-   list_del_init(>list);
-   bnx2fc_process_cq_compl(work->tgt, work->wqe);
-   kfree(work);
-   }
-
-   spin_lock_bh(>fp_work_lock);
-   }
-   __set_current_state(TASK_INTERRUPTIBLE);
+   spin_lock_bh(>fp_work_lock);
+   while (!list_empty(>work_list)) {
+   list_splice_init(>work_list, _list);
spin_unlock_bh(>fp_work_lock);
-   }
-   __set_current_state(TASK_RUNNING);
 
-   return 0;
+   list_for_each_entry_safe(work, tmp, _list, list) {
+   list_del_init(>list);
+   bnx2fc_process_cq_compl(work->tgt, work->wqe);
+   kfree(work);
+   }
+
+   spin_lock_bh(>fp_work_lock);
+   }
+   spin_unlock_bh(>fp_work_lock);
 }
 
 static struct fc_host_statistics *bnx2fc_get_host_stats(struct Scsi_Host 
*shost)
@@ -2571,91 +2557,6 @@ static struct fcoe_transport bnx2fc_transport = {
.disable = bnx2fc_disable,
 };
 
-/**
- * bnx2fc_percpu_thread_crea

Re: [PATCH 1/5] scsi: bnx2i: convert to kworker

[Sebastian Andrzej Siewior]

On 2016-11-18 13:56:22 [+0100], Christoph Hellwig wrote:
> still says kwork in the subject..

I was thinking about this before sending it out. But then it uses the
generic workqueue / kworker and a custom kthread like it did before.
What would you prefer to call it then? Just workqueue?

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/5] scsi: bnx2fc: annoate unlock + release for sparse

[Sebastian Andrzej Siewior]

The caller of bnx2fc_abts_cleanup() holds the tgt->tgt_lock lock and it
is expected to release the lock during wait_for_completion() and acquire
the lock afterwards.

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc_io.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_io.c b/drivers/scsi/bnx2fc/bnx2fc_io.c
index f501095f91ac..634e1e539850 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_io.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_io.c
@@ -1080,6 +1080,8 @@ int bnx2fc_eh_device_reset(struct scsi_cmnd *sc_cmd)
 }
 
 static int bnx2fc_abts_cleanup(struct bnx2fc_cmd *io_req)
+__releases(tgt->tgt_lock)
+__acquires(tgt->tgt_lock)
 {
struct bnx2fc_rport *tgt = io_req->tgt;
int rc = SUCCESS;
-- 
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] scsi: bnx2i: convert to kworker

[Sebastian Andrzej Siewior]

The driver creates its own per-CPU threads which are updated based on CPU
hotplug events. It is also possible to use kworkers and remove some of the
infrastructure get the same job done while saving a few lines of code.

The DECLARE_PER_CPU() definition is moved into the header file where it
belongs. bnx2i_percpu_io_thread() becomes bnx2i_percpu_io_work() which is
mostly the same code. The outer loop (kthread_should_stop()) gets removed and
the remaining code is shifted to the left.
bnx2i_queue_scsi_cmd_resp() is mostly the same. The code checked ->iothread to
decide if there is an active per-CPU thread. With the kworkers this is no
longer possible nor required.
The allocation of struct bnx2i_work does not happen with ->p_work_lock held
which is not required. I am unsure about the call-stack so I can't say
if this qualifies it for the allocation with GFP_KERNEL instead of
GFP_ATOMIC (it is not _bh lock but as I said, I don't know the context).
The allocation case has been reversed so the inner if case is called on
!bnx2i_work and is just the invocation one function since the lock is not
held during allocation. The init of the new bnx2i_work struct is now
done also without the ->p_work_lock held: it is a new object, nobody
knows about it yet. It should be enough to hold the lock while adding
this item to the list. I am unsure about that atomic_inc() so I keep
things as they were.

The remaining part is the removal CPU hotplug notifier since it is taken
care by the kworker code.

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2i/bnx2i.h  |  11 +---
 drivers/scsi/bnx2i/bnx2i_hwi.c  | 101 ++---
 drivers/scsi/bnx2i/bnx2i_init.c | 121 +++-
 3 files changed, 53 insertions(+), 180 deletions(-)

diff --git a/drivers/scsi/bnx2i/bnx2i.h b/drivers/scsi/bnx2i/bnx2i.h
index ed7f3228e234..78cdc493bab5 100644
--- a/drivers/scsi/bnx2i/bnx2i.h
+++ b/drivers/scsi/bnx2i/bnx2i.h
@@ -31,7 +31,6 @@
 #include 
 #include 
 #include 
-#include 
 
 #include 
 #include 
@@ -775,12 +774,11 @@ struct bnx2i_work {
 };
 
 struct bnx2i_percpu_s {
-   struct task_struct *iothread;
+   struct work_struct work;
struct list_head work_list;
spinlock_t p_work_lock;
 };
 
-
 /* Global variables */
 extern unsigned int error_mask1, error_mask2;
 extern u64 iscsi_error_mask;
@@ -797,7 +795,7 @@ extern unsigned int rq_size;
 
 extern struct device_attribute *bnx2i_dev_attributes[];
 
-
+DECLARE_PER_CPU(struct bnx2i_percpu_s, bnx2i_percpu);
 
 /*
  * Function Prototypes
@@ -875,8 +873,5 @@ extern void bnx2i_print_active_cmd_queue(struct bnx2i_conn 
*conn);
 extern void bnx2i_print_xmit_pdu_queue(struct bnx2i_conn *conn);
 extern void bnx2i_print_recv_state(struct bnx2i_conn *conn);
 
-extern int bnx2i_percpu_io_thread(void *arg);
-extern int bnx2i_process_scsi_cmd_resp(struct iscsi_session *session,
-  struct bnx2i_conn *bnx2i_conn,
-  struct cqe *cqe);
+extern void bnx2i_percpu_io_work(struct work_struct *work);
 #endif
diff --git a/drivers/scsi/bnx2i/bnx2i_hwi.c b/drivers/scsi/bnx2i/bnx2i_hwi.c
index 42921dbba927..9be58f6523b3 100644
--- a/drivers/scsi/bnx2i/bnx2i_hwi.c
+++ b/drivers/scsi/bnx2i/bnx2i_hwi.c
@@ -19,8 +19,6 @@
 #include 
 #include "bnx2i.h"
 
-DECLARE_PER_CPU(struct bnx2i_percpu_s, bnx2i_percpu);
-
 /**
  * bnx2i_get_cid_num - get cid from ep
  * @ep:endpoint pointer
@@ -1350,9 +1348,9 @@ int bnx2i_send_fw_iscsi_init_msg(struct bnx2i_hba *hba)
  *
  * process SCSI CMD Response CQE & complete the request to SCSI-ML
  */
-int bnx2i_process_scsi_cmd_resp(struct iscsi_session *session,
-   struct bnx2i_conn *bnx2i_conn,
-   struct cqe *cqe)
+static int bnx2i_process_scsi_cmd_resp(struct iscsi_session *session,
+  struct bnx2i_conn *bnx2i_conn,
+  struct cqe *cqe)
 {
struct iscsi_conn *conn = bnx2i_conn->cls_conn->dd_data;
struct bnx2i_hba *hba = bnx2i_conn->hba;
@@ -1862,45 +1860,37 @@ static void bnx2i_process_cmd_cleanup_resp(struct 
iscsi_session *session,
 
 
 /**
- * bnx2i_percpu_io_thread - thread per cpu for ios
+ * bnx2i_percpu_io_work - thread per cpu for ios
  *
- * @arg:   ptr to bnx2i_percpu_info structure
+ * @work_s:The work struct
  */
-int bnx2i_percpu_io_thread(void *arg)
+void bnx2i_percpu_io_work(struct work_struct *work_s)
 {
-   struct bnx2i_percpu_s *p = arg;
+   struct bnx2i_percpu_s *p;
struct bnx2i_work *work, *tmp;
LIST_HEAD(work_list);
 
-   set_user_nice(current, MIN_NICE);
+   p = container_of(work_s, struct bnx2i_percpu_s, 

[PATCH 3/5] scsi: bnx2fc: clean up header definitions

[Sebastian Andrzej Siewior]

- All symbols which are only used within one .c file are marked static
  and removed from the bnx2fc.h file if possible.

- the declarion of bnx2fc_percpu is moved into the header file

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc.h  |  5 +
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 19 +--
 drivers/scsi/bnx2fc/bnx2fc_hwi.c  |  6 ++
 3 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc.h b/drivers/scsi/bnx2fc/bnx2fc.h
index fdd89935cac7..64308584405d 100644
--- a/drivers/scsi/bnx2fc/bnx2fc.h
+++ b/drivers/scsi/bnx2fc/bnx2fc.h
@@ -172,6 +172,7 @@ struct bnx2fc_percpu_s {
struct list_head work_list;
spinlock_t fp_work_lock;
 };
+DECLARE_PER_CPU(struct bnx2fc_percpu_s, bnx2fc_percpu);
 
 struct bnx2fc_fw_stats {
u64 fc_crc_cnt;
@@ -513,7 +514,6 @@ void bnx2fc_cmd_mgr_free(struct bnx2fc_cmd_mgr *cmgr);
 void bnx2fc_get_link_state(struct bnx2fc_hba *hba);
 char *bnx2fc_get_next_rqe(struct bnx2fc_rport *tgt, u8 num_items);
 void bnx2fc_return_rqe(struct bnx2fc_rport *tgt, u8 num_items);
-int bnx2fc_get_paged_crc_eof(struct sk_buff *skb, int tlen);
 int bnx2fc_send_rrq(struct bnx2fc_cmd *aborted_io_req);
 int bnx2fc_send_adisc(struct bnx2fc_rport *tgt, struct fc_frame *fp);
 int bnx2fc_send_logo(struct bnx2fc_rport *tgt, struct fc_frame *fp);
@@ -537,7 +537,6 @@ void bnx2fc_init_task(struct bnx2fc_cmd *io_req,
 void bnx2fc_add_2_sq(struct bnx2fc_rport *tgt, u16 xid);
 void bnx2fc_ring_doorbell(struct bnx2fc_rport *tgt);
 int bnx2fc_eh_abort(struct scsi_cmnd *sc_cmd);
-int bnx2fc_eh_host_reset(struct scsi_cmnd *sc_cmd);
 int bnx2fc_eh_target_reset(struct scsi_cmnd *sc_cmd);
 int bnx2fc_eh_device_reset(struct scsi_cmnd *sc_cmd);
 void bnx2fc_rport_event_handler(struct fc_lport *lport,
@@ -570,8 +569,6 @@ struct fc_seq *bnx2fc_elsct_send(struct fc_lport *lport, 
u32 did,
   struct fc_frame *,
   void *),
  void *arg, u32 timeout);
-void bnx2fc_arm_cq(struct bnx2fc_rport *tgt);
-int bnx2fc_process_new_cqes(struct bnx2fc_rport *tgt);
 void bnx2fc_process_cq_compl(struct bnx2fc_rport *tgt, u16 wqe);
 struct bnx2fc_rport *bnx2fc_tgt_lookup(struct fcoe_port *port,
 u32 port_id);
diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c 
b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index 79ff0ed2b364..97424e0eaa23 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -49,7 +49,7 @@ struct workqueue_struct *bnx2fc_wq;
  * Here the io threads are per cpu but the l2 thread is just one
  */
 struct fcoe_percpu_s bnx2fc_global;
-DEFINE_SPINLOCK(bnx2fc_global_lock);
+static DEFINE_SPINLOCK(bnx2fc_global_lock);
 
 static struct cnic_ulp_ops bnx2fc_cnic_cb;
 static struct libfc_function_template bnx2fc_libfc_fcn_templ;
@@ -107,22 +107,22 @@ MODULE_PARM_DESC(debug_logging,
"\t\t0x10 - fcoe L2 fame related logs.\n"
"\t\t0xff - LOG all messages.");
 
-uint bnx2fc_devloss_tmo;
+static uint bnx2fc_devloss_tmo;
 module_param_named(devloss_tmo, bnx2fc_devloss_tmo, uint, S_IRUGO);
 MODULE_PARM_DESC(devloss_tmo, " Change devloss_tmo for the remote ports "
"attached via bnx2fc.");
 
-uint bnx2fc_max_luns = BNX2FC_MAX_LUN;
+static uint bnx2fc_max_luns = BNX2FC_MAX_LUN;
 module_param_named(max_luns, bnx2fc_max_luns, uint, S_IRUGO);
 MODULE_PARM_DESC(max_luns, " Change the default max_lun per SCSI host. Default 
"
"0x.");
 
-uint bnx2fc_queue_depth;
+static uint bnx2fc_queue_depth;
 module_param_named(queue_depth, bnx2fc_queue_depth, uint, S_IRUGO);
 MODULE_PARM_DESC(queue_depth, " Change the default queue depth of SCSI devices 
"
"attached via bnx2fc.");
 
-uint bnx2fc_log_fka;
+static uint bnx2fc_log_fka;
 module_param_named(log_fka, bnx2fc_log_fka, uint, S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(log_fka, " Print message to kernel log when fcoe is "
"initiating a FIP keep alive when debug logging is enabled.");
@@ -167,7 +167,7 @@ static void bnx2fc_clean_rx_queue(struct fc_lport *lp)
spin_unlock_bh(>fcoe_rx_list.lock);
 }
 
-int bnx2fc_get_paged_crc_eof(struct sk_buff *skb, int tlen)
+static int bnx2fc_get_paged_crc_eof(struct sk_buff *skb, int tlen)
 {
int rc;
spin_lock(_global_lock);
@@ -1396,10 +1396,9 @@ static struct bnx2fc_hba *bnx2fc_hba_create(struct 
cnic_dev *cnic)
return NULL;
 }
 
-static struct bnx2fc_interface *
-bnx2fc_interface_create(struct bnx2fc_hba *hba,
-   struct net_device *netdev,
-   enum fip_state fip_mode)
+static st

[PATCH 5/5] scsi: bnx2fc: convert bnx2fc_l2_rcv_thread() to worker

[Sebastian Andrzej Siewior]

This is not driven by the hotplug conversation but while I am at it
looks like a good candidate. Converting the thread to a kworker user
removes also the kthread member from struct fcoe_percpu_s.

This driver uses the struct fcoe_percpu_s but it does not need the
crc_eof_page member, only the work item and fcoe_rx_list. So it is
removed there.

We had one thread so we only use the kworker on the current CPU. If
someone knows how spread this nicely, it would only require the usage of
schedule_work_on() instead schedule_work() :)

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Cc: fcoe-de...@open-fcoe.org
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 57 +--
 include/scsi/libfcoe.h|  1 -
 2 files changed, 12 insertions(+), 46 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c 
b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index 97424e0eaa23..698348b9fa99 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -479,7 +479,7 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct 
net_device *dev,
 
__skb_queue_tail(>fcoe_rx_list, skb);
if (bg->fcoe_rx_list.qlen == 1)
-   wake_up_process(bg->kthread);
+   schedule_work(>work);
 
spin_unlock(>fcoe_rx_list.lock);
 
@@ -489,26 +489,20 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct 
net_device *dev,
return -1;
 }
 
-static int bnx2fc_l2_rcv_thread(void *arg)
+static void bnx2fc_l2_rcv_work(struct work_struct *work_s)
 {
-   struct fcoe_percpu_s *bg = arg;
+   struct fcoe_percpu_s *bg;
struct sk_buff *skb;
 
-   set_user_nice(current, MIN_NICE);
-   set_current_state(TASK_INTERRUPTIBLE);
-   while (!kthread_should_stop()) {
-   schedule();
-   spin_lock_bh(>fcoe_rx_list.lock);
-   while ((skb = __skb_dequeue(>fcoe_rx_list)) != NULL) {
-   spin_unlock_bh(>fcoe_rx_list.lock);
-   bnx2fc_recv_frame(skb);
-   spin_lock_bh(>fcoe_rx_list.lock);
-   }
-   __set_current_state(TASK_INTERRUPTIBLE);
+   bg = container_of(work_s, struct fcoe_percpu_s, work);
+
+   spin_lock_bh(>fcoe_rx_list.lock);
+   while ((skb = __skb_dequeue(>fcoe_rx_list)) != NULL) {
spin_unlock_bh(>fcoe_rx_list.lock);
+   bnx2fc_recv_frame(skb);
+   spin_lock_bh(>fcoe_rx_list.lock);
}
-   __set_current_state(TASK_RUNNING);
-   return 0;
+   spin_unlock_bh(>fcoe_rx_list.lock);
 }
 
 
@@ -2574,7 +2568,6 @@ static int bnx2fc_slave_configure(struct scsi_device 
*sdev)
 static int __init bnx2fc_mod_init(void)
 {
struct fcoe_percpu_s *bg;
-   struct task_struct *l2_thread;
int rc = 0;
unsigned int cpu = 0;
struct bnx2fc_percpu_s *p;
@@ -2607,17 +2600,7 @@ static int __init bnx2fc_mod_init(void)
 
bg = _global;
skb_queue_head_init(>fcoe_rx_list);
-   l2_thread = kthread_create(bnx2fc_l2_rcv_thread,
-  (void *)bg,
-  "bnx2fc_l2_thread");
-   if (IS_ERR(l2_thread)) {
-   rc = PTR_ERR(l2_thread);
-   goto free_wq;
-   }
-   wake_up_process(l2_thread);
-   spin_lock_bh(>fcoe_rx_list.lock);
-   bg->kthread = l2_thread;
-   spin_unlock_bh(>fcoe_rx_list.lock);
+   INIT_WORK(>work, bnx2fc_l2_rcv_work);
 
for_each_possible_cpu(cpu) {
p = _cpu(bnx2fc_percpu, cpu);
@@ -2630,8 +2613,6 @@ static int __init bnx2fc_mod_init(void)
 
return 0;
 
-free_wq:
-   destroy_workqueue(bnx2fc_wq);
 release_bt:
bnx2fc_release_transport();
 detach_ft:
@@ -2644,9 +2625,6 @@ static void __exit bnx2fc_mod_exit(void)
 {
LIST_HEAD(to_be_deleted);
struct bnx2fc_hba *hba, *next;
-   struct fcoe_percpu_s *bg;
-   struct task_struct *l2_thread;
-   struct sk_buff *skb;
unsigned int cpu = 0;
 
/*
@@ -2675,18 +2653,7 @@ static void __exit bnx2fc_mod_exit(void)
}
cnic_unregister_driver(CNIC_ULP_FCOE);
 
-   /* Destroy global thread */
-   bg = _global;
-   spin_lock_bh(>fcoe_rx_list.lock);
-   l2_thread = bg->kthread;
-   bg->kthread = NULL;
-   while ((skb = __skb_dequeue(>fcoe_rx_list)) != NULL)
-   kfree_skb(skb);
-
-   spin_unlock_bh(>fcoe_rx_list.lock);
-
-   if (l2_thread)
-   kthread_stop(l2_thread);
+   flush_work(_global.work);
 
for_each_possible_cpu(cpu) {
struct bnx2fc_percpu_s *p;
diff --git a/include/scsi/libfcoe.h b/include/scsi/libfcoe.h
index 722d3264d3bf..ca6fb3c0f172 100644
--- a

bnx2i + bnx2fc: convert to generic workqueue

[Sebastian Andrzej Siewior]

This is the second repost + fixups to get the patches applied against v4.9-rc4.
mkp's scsi for-next tree can be merged with no conflicts.

The last repost was not merged and stalled after Chad Dupuis said to hold on
because he is testing but never came back with the results [0]. Johannes
confirmed that he did some testing of bnx2fc but he is not sure about bnx2i
[1]. hch asked later about something different but wanted them in since they
were tested as-is [2].

The whole series is also available at
 git://git.kernel.org/pub/scm/linux/kernel/git/bigeasy/hotplug-staging.git 
scsi/bnx2

[0] https://lkml.kernel.org/r/yq137l2mm13@sermon.lab.mkp.net
[1] https://lkml.kernel.org/r/20161108072434.gnnhmrkkmxroy...@linux-x5ow.site
[2] https://lkml.kernel.org/r/20161107191025.ga5...@lst.de

Sebastian Andrzej Siewior (5):
  scsi: bnx2i: convert to kworker
  scsi: bnx2fc: convert per-CPU thread to kworker
  scsi: bnx2fc: clean up header definitions
  scsi: bnx2fc: annoate unlock + release for sparse
  scsi: bnx2fc: convert bnx2fc_l2_rcv_thread() to worker

 drivers/scsi/bnx2fc/bnx2fc.h  |   7 +-
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 233 +++---
 drivers/scsi/bnx2fc/bnx2fc_hwi.c  |  28 ++---
 drivers/scsi/bnx2fc/bnx2fc_io.c   |   2 +
 drivers/scsi/bnx2i/bnx2i.h|  11 +-
 drivers/scsi/bnx2i/bnx2i_hwi.c| 101 +++--
 drivers/scsi/bnx2i/bnx2i_init.c   | 121 ++--
 include/scsi/libfcoe.h|   1 -
 8 files changed, 111 insertions(+), 393 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] scsi: bnx2fc: convert per-CPU thread to kworker

[Sebastian Andrzej Siewior]

The driver creates its own per-CPU threads which are updated based on CPU
hotplug events. It is also possible to use kworkers and remove some of the
infrastructure get the same job done while saving a few lines of code.

bnx2fc_percpu_io_thread() becomes bnx2fc_percpu_io_work() which is
mostly the same code. The outer loop (kthread_should_stop()) gets
removed and the remaining code is shifted to the left.
In bnx2fc_process_new_cqes() the code checked for ->iothread to
decide if there is an active per-CPU thread. With the kworkers this
is no longer possible nor required. The allocation of a new work item
(via bnx2fc_alloc_work()) does no longer happen with the ->fp_work_lock
lock held. It performs only a memory allocation + initialization which
does not require any kind of serialization. The lock is held while
adding the new member to fps->work_list list.

The remaining part is the removal CPU hotplug notifier since it is taken
care by the kworker code.

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc.h  |   2 +-
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 157 ++
 drivers/scsi/bnx2fc/bnx2fc_hwi.c  |  22 +++---
 3 files changed, 32 insertions(+), 149 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc.h b/drivers/scsi/bnx2fc/bnx2fc.h
index fdd4eb4e41b2..fdd89935cac7 100644
--- a/drivers/scsi/bnx2fc/bnx2fc.h
+++ b/drivers/scsi/bnx2fc/bnx2fc.h
@@ -168,7 +168,7 @@ extern struct fcoe_percpu_s bnx2fc_global;
 extern struct workqueue_struct *bnx2fc_wq;
 
 struct bnx2fc_percpu_s {
-   struct task_struct *iothread;
+   struct work_struct work;
struct list_head work_list;
spinlock_t fp_work_lock;
 };
diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c 
b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index f9ddb6156f14..79ff0ed2b364 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -127,13 +127,6 @@ module_param_named(log_fka, bnx2fc_log_fka, uint, 
S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(log_fka, " Print message to kernel log when fcoe is "
"initiating a FIP keep alive when debug logging is enabled.");
 
-static int bnx2fc_cpu_callback(struct notifier_block *nfb,
-unsigned long action, void *hcpu);
-/* notification function for CPU hotplug events */
-static struct notifier_block bnx2fc_cpu_notifier = {
-   .notifier_call = bnx2fc_cpu_callback,
-};
-
 static inline struct net_device *bnx2fc_netdev(const struct fc_lport *lport)
 {
return ((struct bnx2fc_interface *)
@@ -621,39 +614,32 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
 }
 
 /**
- * bnx2fc_percpu_io_thread - thread per cpu for ios
+ * bnx2fc_percpu_io_work - work per cpu for ios
  *
- * @arg:   ptr to bnx2fc_percpu_info structure
+ * @work_s:The work struct
  */
-static int bnx2fc_percpu_io_thread(void *arg)
+static void bnx2fc_percpu_io_work(struct work_struct *work_s)
 {
-   struct bnx2fc_percpu_s *p = arg;
+   struct bnx2fc_percpu_s *p;
struct bnx2fc_work *work, *tmp;
LIST_HEAD(work_list);
 
-   set_user_nice(current, MIN_NICE);
-   set_current_state(TASK_INTERRUPTIBLE);
-   while (!kthread_should_stop()) {
-   schedule();
-   spin_lock_bh(>fp_work_lock);
-   while (!list_empty(>work_list)) {
-   list_splice_init(>work_list, _list);
-   spin_unlock_bh(>fp_work_lock);
+   p = container_of(work_s, struct bnx2fc_percpu_s, work);
 
-   list_for_each_entry_safe(work, tmp, _list, list) {
-   list_del_init(>list);
-   bnx2fc_process_cq_compl(work->tgt, work->wqe);
-   kfree(work);
-   }
-
-   spin_lock_bh(>fp_work_lock);
-   }
-   __set_current_state(TASK_INTERRUPTIBLE);
+   spin_lock_bh(>fp_work_lock);
+   while (!list_empty(>work_list)) {
+   list_splice_init(>work_list, _list);
spin_unlock_bh(>fp_work_lock);
-   }
-   __set_current_state(TASK_RUNNING);
 
-   return 0;
+   list_for_each_entry_safe(work, tmp, _list, list) {
+   list_del_init(>list);
+   bnx2fc_process_cq_compl(work->tgt, work->wqe);
+   kfree(work);
+   }
+
+   spin_lock_bh(>fp_work_lock);
+   }
+   spin_unlock_bh(>fp_work_lock);
 }
 
 static struct fc_host_statistics *bnx2fc_get_host_stats(struct Scsi_Host 
*shost)
@@ -2571,91 +2557,6 @@ static struct fcoe_transport bnx2fc_transport = {
.disable = bnx2fc_disable,
 };
 
-/**
- * bnx2fc_percpu_thread_crea

Re: [PREEMPT-RT] [REPOST PATCH 1/5] scsi: bnx2i: convert to kworker

[Sebastian Andrzej Siewior]

On 2016-11-07 17:48:46 [+0100], Christoph Hellwig wrote:
> On Mon, Nov 07, 2016 at 05:46:29PM +0100, Sebastian Andrzej Siewior wrote:
> > sorry for the confusion in the subject. If I remember correctly you said
> > that we may not have enough room for a larger / work_struct struct and I
> > should keep the list for now.
> 
> But that was for libfc where it only has a tiny bit of private data
> i nthe skb - this is for cnix which has plenty of space, and in fact

Right. now I remember.

> probably should do the workqueue offload in the core before invoking
> the protocol drivers.

So we keep things as they are right now or are we getting also rid of
the internal list? This was tested by Johannes and Chad (claimed to do
testing) and the removal the internal list afterwards shouldn't be that
big deal. Not sure what the last point is about.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PREEMPT-RT] [REPOST PATCH 1/5] scsi: bnx2i: convert to kworker

[Sebastian Andrzej Siewior]

On 2016-11-07 17:38:48 [+0100], Christoph Hellwig wrote:
> You're right it does - between the incorrect subject and the fact
> that it still keeps the linked list of items arounds instead of fully
> using the workqueue infrastructure I was a bit confused before my
> first coffee this morning.  Same applies to patch 2.

sorry for the confusion in the subject. If I remember correctly you said
that we may not have enough room for a larger / work_struct struct and I
should keep the list for now.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [REPOST PATCH 1/5] scsi: bnx2i: convert to kworker

[Sebastian Andrzej Siewior]

On 2016-09-14 13:25:12 [-0400], Martin K. Petersen wrote:
> > "Chad" == Chad Dupuis  writes:
> 
> Chad> We're regression testing the patches now.  Please hold off on
> Chad> applying them.
> 
> OK. I will wait.

Chad, any updates on this tests?

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 10/21] virtio scsi: Convert to hotplug state machine

[Sebastian Andrzej Siewior]

Install the callbacks via the state machine. It uses the multi instance
infrastructure of the hotplug code to handle each interface.

virtscsi_set_affinity() is removed from virtscsi_init() because
virtscsi_cpu_notif_add() (the function which registers the instance) is invoked
right after it and the cpuhp_state_add_instance() functions invokes the startup
callback on all online CPUs.

The same thing can not be applied virtscsi_cpu_notif_remove() because
virtscsi_remove_vqs() invokes virtscsi_set_affinity() with affinity = false as
argument but the old CPU_DEAD state invoked the function with affinity = true
(which does not match the DEAD callback).

Cc: "James E.J. Bottomley" <j...@linux.vnet.ibm.com>
Cc: "Martin K. Petersen" <martin.peter...@oracle.com>
Cc: linux-scsi@vger.kernel.org
Cc: "Michael S. Tsirkin" <m...@redhat.com>
Cc: virtualizat...@lists.linux-foundation.org
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/virtio_scsi.c | 76 ++
 include/linux/cpuhotplug.h |  1 +
 2 files changed, 50 insertions(+), 27 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 7dbbb29d24c6..deefab3a94d0 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -107,8 +107,8 @@ struct virtio_scsi {
/* If the affinity hint is set for virtqueues */
bool affinity_hint_set;
 
-   /* CPU hotplug notifier */
-   struct notifier_block nb;
+   struct hlist_node node;
+   struct hlist_node node_dead;
 
/* Protected by event_vq lock */
bool stop_events;
@@ -118,6 +118,7 @@ struct virtio_scsi {
struct virtio_scsi_vq req_vqs[];
 };
 
+static enum cpuhp_state virtioscsi_online;
 static struct kmem_cache *virtscsi_cmd_cache;
 static mempool_t *virtscsi_cmd_pool;
 
@@ -852,21 +853,33 @@ static void virtscsi_set_affinity(struct virtio_scsi 
*vscsi, bool affinity)
put_online_cpus();
 }
 
-static int virtscsi_cpu_callback(struct notifier_block *nfb,
-unsigned long action, void *hcpu)
+static int virtscsi_cpu_online(unsigned int cpu, struct hlist_node *node)
 {
-   struct virtio_scsi *vscsi = container_of(nfb, struct virtio_scsi, nb);
-   switch(action) {
-   case CPU_ONLINE:
-   case CPU_ONLINE_FROZEN:
-   case CPU_DEAD:
-   case CPU_DEAD_FROZEN:
-   __virtscsi_set_affinity(vscsi, true);
-   break;
-   default:
-   break;
-   }
-   return NOTIFY_OK;
+   struct virtio_scsi *vscsi = hlist_entry_safe(node, struct virtio_scsi,
+node);
+   __virtscsi_set_affinity(vscsi, true);
+   return 0;
+}
+
+static int virtscsi_cpu_notif_add(struct virtio_scsi *vi)
+{
+   int ret;
+
+   ret = cpuhp_state_add_instance(virtioscsi_online, >node);
+   if (ret)
+   return ret;
+
+   ret = cpuhp_state_add_instance(CPUHP_VIRT_SCSI_DEAD, >node_dead);
+   if (ret)
+   cpuhp_state_remove_instance(virtioscsi_online, >node);
+   return ret;
+}
+
+static void virtscsi_cpu_notif_remove(struct virtio_scsi *vi)
+{
+   cpuhp_state_remove_instance_nocalls(virtioscsi_online, >node);
+   cpuhp_state_remove_instance_nocalls(CPUHP_VIRT_SCSI_DEAD,
+   >node_dead);
 }
 
 static void virtscsi_init_vq(struct virtio_scsi_vq *virtscsi_vq,
@@ -929,8 +942,6 @@ static int virtscsi_init(struct virtio_device *vdev,
virtscsi_init_vq(>req_vqs[i - VIRTIO_SCSI_VQ_BASE],
 vqs[i]);
 
-   virtscsi_set_affinity(vscsi, true);
-
virtscsi_config_set(vdev, cdb_size, VIRTIO_SCSI_CDB_SIZE);
virtscsi_config_set(vdev, sense_size, VIRTIO_SCSI_SENSE_SIZE);
 
@@ -987,12 +998,9 @@ static int virtscsi_probe(struct virtio_device *vdev)
if (err)
goto virtscsi_init_failed;
 
-   vscsi->nb.notifier_call = _cpu_callback;
-   err = register_hotcpu_notifier(>nb);
-   if (err) {
-   pr_err("registering cpu notifier failed\n");
+   err = virtscsi_cpu_notif_add(vscsi);
+   if (err)
goto scsi_add_host_failed;
-   }
 
cmd_per_lun = virtscsi_config_get(vdev, cmd_per_lun) ?: 1;
shost->cmd_per_lun = min_t(u32, cmd_per_lun, shost->can_queue);
@@ -1049,7 +1057,7 @@ static void virtscsi_remove(struct virtio_device *vdev)
 
scsi_remove_host(shost);
 
-   unregister_hotcpu_notifier(>nb);
+   virtscsi_cpu_notif_remove(vscsi);
 
virtscsi_remove_vqs(vdev);
scsi_host_put(shost);
@@ -1061,7 +1069,7 @@ static int virtscsi_freeze(struct virtio_device *vdev)
struct Scsi_Host *sh = virtio_scsi_host(vdev);
struct virtio_scsi *vscsi = shost_priv(sh);
 
-   unregister

[REPOST PATCH 2/5] scsi: bnx2fc: convert per-CPU thread to kworker

[Sebastian Andrzej Siewior]

The driver creates its own per-CPU threads which are updated based on CPU
hotplug events. It is also possible to use kworkers and remove some of the
infrastructure get the same job done while saving a few lines of code.

bnx2fc_percpu_io_thread() becomes bnx2fc_percpu_io_work() which is
mostly the same code. The outer loop (kthread_should_stop()) gets
removed and the remaining code is shifted to the left.
In bnx2fc_process_new_cqes() the code checked for ->iothread to
decide if there is an active per-CPU thread. With the kworkers this
is no longer possible nor required. The allocation of a new work item
(via bnx2fc_alloc_work()) does no longer happen with the ->fp_work_lock
lock held. It performs only a memory allocation + initialization which
does not require any kind of serialization. The lock is held while
adding the new member to fps->work_list list.

The remaining part is the removal CPU hotplug notifier since it is taken
care by the kworker code.

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc.h  |   2 +-
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 157 ++
 drivers/scsi/bnx2fc/bnx2fc_hwi.c  |  22 +++---
 3 files changed, 32 insertions(+), 149 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc.h b/drivers/scsi/bnx2fc/bnx2fc.h
index fdd4eb4e41b2..fdd89935cac7 100644
--- a/drivers/scsi/bnx2fc/bnx2fc.h
+++ b/drivers/scsi/bnx2fc/bnx2fc.h
@@ -168,7 +168,7 @@ extern struct fcoe_percpu_s bnx2fc_global;
 extern struct workqueue_struct *bnx2fc_wq;
 
 struct bnx2fc_percpu_s {
-   struct task_struct *iothread;
+   struct work_struct work;
struct list_head work_list;
spinlock_t fp_work_lock;
 };
diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c 
b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index a5052dd8d7e6..c8673e3c4d35 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -127,13 +127,6 @@ module_param_named(log_fka, bnx2fc_log_fka, uint, 
S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(log_fka, " Print message to kernel log when fcoe is "
"initiating a FIP keep alive when debug logging is enabled.");
 
-static int bnx2fc_cpu_callback(struct notifier_block *nfb,
-unsigned long action, void *hcpu);
-/* notification function for CPU hotplug events */
-static struct notifier_block bnx2fc_cpu_notifier = {
-   .notifier_call = bnx2fc_cpu_callback,
-};
-
 static inline struct net_device *bnx2fc_netdev(const struct fc_lport *lport)
 {
return ((struct bnx2fc_interface *)
@@ -621,39 +614,32 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
 }
 
 /**
- * bnx2fc_percpu_io_thread - thread per cpu for ios
+ * bnx2fc_percpu_io_work - work per cpu for ios
  *
- * @arg:   ptr to bnx2fc_percpu_info structure
+ * @work_s:The work struct
  */
-int bnx2fc_percpu_io_thread(void *arg)
+static void bnx2fc_percpu_io_work(struct work_struct *work_s)
 {
-   struct bnx2fc_percpu_s *p = arg;
+   struct bnx2fc_percpu_s *p;
struct bnx2fc_work *work, *tmp;
LIST_HEAD(work_list);
 
-   set_user_nice(current, MIN_NICE);
-   set_current_state(TASK_INTERRUPTIBLE);
-   while (!kthread_should_stop()) {
-   schedule();
-   spin_lock_bh(>fp_work_lock);
-   while (!list_empty(>work_list)) {
-   list_splice_init(>work_list, _list);
-   spin_unlock_bh(>fp_work_lock);
+   p = container_of(work_s, struct bnx2fc_percpu_s, work);
 
-   list_for_each_entry_safe(work, tmp, _list, list) {
-   list_del_init(>list);
-   bnx2fc_process_cq_compl(work->tgt, work->wqe);
-   kfree(work);
-   }
-
-   spin_lock_bh(>fp_work_lock);
-   }
-   __set_current_state(TASK_INTERRUPTIBLE);
+   spin_lock_bh(>fp_work_lock);
+   while (!list_empty(>work_list)) {
+   list_splice_init(>work_list, _list);
spin_unlock_bh(>fp_work_lock);
-   }
-   __set_current_state(TASK_RUNNING);
 
-   return 0;
+   list_for_each_entry_safe(work, tmp, _list, list) {
+   list_del_init(>list);
+   bnx2fc_process_cq_compl(work->tgt, work->wqe);
+   kfree(work);
+   }
+
+   spin_lock_bh(>fp_work_lock);
+   }
+   spin_unlock_bh(>fp_work_lock);
 }
 
 static struct fc_host_statistics *bnx2fc_get_host_stats(struct Scsi_Host 
*shost)
@@ -2570,91 +2556,6 @@ static struct fcoe_transport bnx2fc_transport = {
.disable = bnx2fc_disable,
 };
 
-/**
- * bnx2fc_percpu_thread_create - 

[REPOST PATCH 1/5] scsi: bnx2i: convert to kworker

[Sebastian Andrzej Siewior]

The driver creates its own per-CPU threads which are updated based on CPU
hotplug events. It is also possible to use kworkers and remove some of the
infrastructure get the same job done while saving a few lines of code.

The DECLARE_PER_CPU() definition is moved into the header file where it
belongs. bnx2i_percpu_io_thread() becomes bnx2i_percpu_io_work() which is
mostly the same code. The outer loop (kthread_should_stop()) gets removed and
the remaining code is shifted to the left.
bnx2i_queue_scsi_cmd_resp() is mostly the same. The code checked ->iothread to
decide if there is an active per-CPU thread. With the kworkers this is no
longer possible nor required.
The allocation of struct bnx2i_work does not happen with ->p_work_lock held
which is not required. I am unsure about the call-stack so I can't say
if this qualifies it for the allocation with GFP_KERNEL instead of
GFP_ATOMIC (it is not _bh lock but as I said, I don't know the context).
The allocation case has been reversed so the inner if case is called on
!bnx2i_work and is just the invocation one function since the lock is not
held during allocation. The init of the new bnx2i_work struct is now
done also without the ->p_work_lock held: it is a new object, nobody
knows about it yet. It should be enough to hold the lock while adding
this item to the list. I am unsure about that atomic_inc() so I keep
things as they were.

The remaining part is the removal CPU hotplug notifier since it is taken
care by the kworker code.

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2i/bnx2i.h  |  11 +---
 drivers/scsi/bnx2i/bnx2i_hwi.c  | 101 ++---
 drivers/scsi/bnx2i/bnx2i_init.c | 121 +++-
 3 files changed, 53 insertions(+), 180 deletions(-)

diff --git a/drivers/scsi/bnx2i/bnx2i.h b/drivers/scsi/bnx2i/bnx2i.h
index ed7f3228e234..78cdc493bab5 100644
--- a/drivers/scsi/bnx2i/bnx2i.h
+++ b/drivers/scsi/bnx2i/bnx2i.h
@@ -31,7 +31,6 @@
 #include 
 #include 
 #include 
-#include 
 
 #include 
 #include 
@@ -775,12 +774,11 @@ struct bnx2i_work {
 };
 
 struct bnx2i_percpu_s {
-   struct task_struct *iothread;
+   struct work_struct work;
struct list_head work_list;
spinlock_t p_work_lock;
 };
 
-
 /* Global variables */
 extern unsigned int error_mask1, error_mask2;
 extern u64 iscsi_error_mask;
@@ -797,7 +795,7 @@ extern unsigned int rq_size;
 
 extern struct device_attribute *bnx2i_dev_attributes[];
 
-
+DECLARE_PER_CPU(struct bnx2i_percpu_s, bnx2i_percpu);
 
 /*
  * Function Prototypes
@@ -875,8 +873,5 @@ extern void bnx2i_print_active_cmd_queue(struct bnx2i_conn 
*conn);
 extern void bnx2i_print_xmit_pdu_queue(struct bnx2i_conn *conn);
 extern void bnx2i_print_recv_state(struct bnx2i_conn *conn);
 
-extern int bnx2i_percpu_io_thread(void *arg);
-extern int bnx2i_process_scsi_cmd_resp(struct iscsi_session *session,
-  struct bnx2i_conn *bnx2i_conn,
-  struct cqe *cqe);
+extern void bnx2i_percpu_io_work(struct work_struct *work);
 #endif
diff --git a/drivers/scsi/bnx2i/bnx2i_hwi.c b/drivers/scsi/bnx2i/bnx2i_hwi.c
index 42921dbba927..9be58f6523b3 100644
--- a/drivers/scsi/bnx2i/bnx2i_hwi.c
+++ b/drivers/scsi/bnx2i/bnx2i_hwi.c
@@ -19,8 +19,6 @@
 #include 
 #include "bnx2i.h"
 
-DECLARE_PER_CPU(struct bnx2i_percpu_s, bnx2i_percpu);
-
 /**
  * bnx2i_get_cid_num - get cid from ep
  * @ep:endpoint pointer
@@ -1350,9 +1348,9 @@ int bnx2i_send_fw_iscsi_init_msg(struct bnx2i_hba *hba)
  *
  * process SCSI CMD Response CQE & complete the request to SCSI-ML
  */
-int bnx2i_process_scsi_cmd_resp(struct iscsi_session *session,
-   struct bnx2i_conn *bnx2i_conn,
-   struct cqe *cqe)
+static int bnx2i_process_scsi_cmd_resp(struct iscsi_session *session,
+  struct bnx2i_conn *bnx2i_conn,
+  struct cqe *cqe)
 {
struct iscsi_conn *conn = bnx2i_conn->cls_conn->dd_data;
struct bnx2i_hba *hba = bnx2i_conn->hba;
@@ -1862,45 +1860,37 @@ static void bnx2i_process_cmd_cleanup_resp(struct 
iscsi_session *session,
 
 
 /**
- * bnx2i_percpu_io_thread - thread per cpu for ios
+ * bnx2i_percpu_io_work - thread per cpu for ios
  *
- * @arg:   ptr to bnx2i_percpu_info structure
+ * @work_s:The work struct
  */
-int bnx2i_percpu_io_thread(void *arg)
+void bnx2i_percpu_io_work(struct work_struct *work_s)
 {
-   struct bnx2i_percpu_s *p = arg;
+   struct bnx2i_percpu_s *p;
struct bnx2i_work *work, *tmp;
LIST_HEAD(work_list);
 
-   set_user_nice(current, MIN_NICE);
+   p = container_of(work_s, struct bnx2i_percpu_s, 

[REPOST PATCH 3/5] scsi: bnx2fc: clean up header definitions

[Sebastian Andrzej Siewior]

- All symbols which are only used within one .c file are marked static
  and removed from the bnx2fc.h file if possible.

- the declarion of bnx2fc_percpu is moved into the header file

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc.h  |  5 +
 drivers/scsi/bnx2fc/bnx2fc_els.c  |  4 ++--
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 14 +++---
 drivers/scsi/bnx2fc/bnx2fc_hwi.c  |  8 +++-
 drivers/scsi/bnx2fc/bnx2fc_io.c   |  2 +-
 5 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc.h b/drivers/scsi/bnx2fc/bnx2fc.h
index fdd89935cac7..64308584405d 100644
--- a/drivers/scsi/bnx2fc/bnx2fc.h
+++ b/drivers/scsi/bnx2fc/bnx2fc.h
@@ -172,6 +172,7 @@ struct bnx2fc_percpu_s {
struct list_head work_list;
spinlock_t fp_work_lock;
 };
+DECLARE_PER_CPU(struct bnx2fc_percpu_s, bnx2fc_percpu);
 
 struct bnx2fc_fw_stats {
u64 fc_crc_cnt;
@@ -513,7 +514,6 @@ void bnx2fc_cmd_mgr_free(struct bnx2fc_cmd_mgr *cmgr);
 void bnx2fc_get_link_state(struct bnx2fc_hba *hba);
 char *bnx2fc_get_next_rqe(struct bnx2fc_rport *tgt, u8 num_items);
 void bnx2fc_return_rqe(struct bnx2fc_rport *tgt, u8 num_items);
-int bnx2fc_get_paged_crc_eof(struct sk_buff *skb, int tlen);
 int bnx2fc_send_rrq(struct bnx2fc_cmd *aborted_io_req);
 int bnx2fc_send_adisc(struct bnx2fc_rport *tgt, struct fc_frame *fp);
 int bnx2fc_send_logo(struct bnx2fc_rport *tgt, struct fc_frame *fp);
@@ -537,7 +537,6 @@ void bnx2fc_init_task(struct bnx2fc_cmd *io_req,
 void bnx2fc_add_2_sq(struct bnx2fc_rport *tgt, u16 xid);
 void bnx2fc_ring_doorbell(struct bnx2fc_rport *tgt);
 int bnx2fc_eh_abort(struct scsi_cmnd *sc_cmd);
-int bnx2fc_eh_host_reset(struct scsi_cmnd *sc_cmd);
 int bnx2fc_eh_target_reset(struct scsi_cmnd *sc_cmd);
 int bnx2fc_eh_device_reset(struct scsi_cmnd *sc_cmd);
 void bnx2fc_rport_event_handler(struct fc_lport *lport,
@@ -570,8 +569,6 @@ struct fc_seq *bnx2fc_elsct_send(struct fc_lport *lport, 
u32 did,
   struct fc_frame *,
   void *),
  void *arg, u32 timeout);
-void bnx2fc_arm_cq(struct bnx2fc_rport *tgt);
-int bnx2fc_process_new_cqes(struct bnx2fc_rport *tgt);
 void bnx2fc_process_cq_compl(struct bnx2fc_rport *tgt, u16 wqe);
 struct bnx2fc_rport *bnx2fc_tgt_lookup(struct fcoe_port *port,
 u32 port_id);
diff --git a/drivers/scsi/bnx2fc/bnx2fc_els.c b/drivers/scsi/bnx2fc/bnx2fc_els.c
index 5beea776b9f5..68ca518d34b0 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_els.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_els.c
@@ -254,7 +254,7 @@ int bnx2fc_send_rls(struct bnx2fc_rport *tgt, struct 
fc_frame *fp)
return rc;
 }
 
-void bnx2fc_srr_compl(struct bnx2fc_els_cb_arg *cb_arg)
+static void bnx2fc_srr_compl(struct bnx2fc_els_cb_arg *cb_arg)
 {
struct bnx2fc_mp_req *mp_req;
struct fc_frame_header *fc_hdr, *fh;
@@ -364,7 +364,7 @@ void bnx2fc_srr_compl(struct bnx2fc_els_cb_arg *cb_arg)
kref_put(_io_req->refcount, bnx2fc_cmd_release);
 }
 
-void bnx2fc_rec_compl(struct bnx2fc_els_cb_arg *cb_arg)
+static void bnx2fc_rec_compl(struct bnx2fc_els_cb_arg *cb_arg)
 {
struct bnx2fc_cmd *orig_io_req, *new_io_req;
struct bnx2fc_cmd *rec_req;
diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c 
b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index c8673e3c4d35..f2be89984990 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -49,7 +49,7 @@ struct workqueue_struct *bnx2fc_wq;
  * Here the io threads are per cpu but the l2 thread is just one
  */
 struct fcoe_percpu_s bnx2fc_global;
-DEFINE_SPINLOCK(bnx2fc_global_lock);
+static DEFINE_SPINLOCK(bnx2fc_global_lock);
 
 static struct cnic_ulp_ops bnx2fc_cnic_cb;
 static struct libfc_function_template bnx2fc_libfc_fcn_templ;
@@ -107,22 +107,22 @@ MODULE_PARM_DESC(debug_logging,
"\t\t0x10 - fcoe L2 fame related logs.\n"
"\t\t0xff - LOG all messages.");
 
-uint bnx2fc_devloss_tmo;
+static uint bnx2fc_devloss_tmo;
 module_param_named(devloss_tmo, bnx2fc_devloss_tmo, uint, S_IRUGO);
 MODULE_PARM_DESC(devloss_tmo, " Change devloss_tmo for the remote ports "
"attached via bnx2fc.");
 
-uint bnx2fc_max_luns = BNX2FC_MAX_LUN;
+static uint bnx2fc_max_luns = BNX2FC_MAX_LUN;
 module_param_named(max_luns, bnx2fc_max_luns, uint, S_IRUGO);
 MODULE_PARM_DESC(max_luns, " Change the default max_lun per SCSI host. Default 
"
"0x.");
 
-uint bnx2fc_queue_depth;
+static uint bnx2fc_queue_depth;
 module_param_named(queue_depth, bnx2fc_queue_depth, uint, S_IRUGO);
 MODULE_PARM_DESC(queue_depth, " Change the default queue depth of SCSI 

[REPOST PATCH 4/5] scsi: bnx2fc: annoate unlock + release for sparse

[Sebastian Andrzej Siewior]

The caller of bnx2fc_abts_cleanup() holds the tgt->tgt_lock lock and it
is expected to release the lock during wait_for_completion() and acquire
the lock afterwards.

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc_io.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_io.c b/drivers/scsi/bnx2fc/bnx2fc_io.c
index f501095f91ac..634e1e539850 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_io.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_io.c
@@ -1080,6 +1080,8 @@ int bnx2fc_eh_device_reset(struct scsi_cmnd *sc_cmd)
 }
 
 static int bnx2fc_abts_cleanup(struct bnx2fc_cmd *io_req)
+__releases(tgt->tgt_lock)
+__acquires(tgt->tgt_lock)
 {
struct bnx2fc_rport *tgt = io_req->tgt;
int rc = SUCCESS;
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[REPOST PATCH 5/5] scsi: bnx2fc: convert bnx2fc_l2_rcv_thread() to worker

[Sebastian Andrzej Siewior]

This is not driven by the hotplug conversation but while I am at it
looks like a good candidate. Converting the thread to a kworker user
removes also the kthread member from struct fcoe_percpu_s.

This driver uses the struct fcoe_percpu_s but it does not need the
crc_eof_page member, only the work item and fcoe_rx_list. So it is
removed there.

We had one thread so we only use the kworker on the current CPU. If
someone knows how spread this nicely, it would only require the usage of
schedule_work_on() instead schedule_work() :)

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Cc: fcoe-de...@open-fcoe.org
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 57 +--
 include/scsi/libfcoe.h|  1 -
 2 files changed, 12 insertions(+), 46 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c 
b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index f2be89984990..64d1ed0e7c5f 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -479,7 +479,7 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct 
net_device *dev,
 
__skb_queue_tail(>fcoe_rx_list, skb);
if (bg->fcoe_rx_list.qlen == 1)
-   wake_up_process(bg->kthread);
+   schedule_work(>work);
 
spin_unlock(>fcoe_rx_list.lock);
 
@@ -489,26 +489,20 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct 
net_device *dev,
return -1;
 }
 
-static int bnx2fc_l2_rcv_thread(void *arg)
+static void bnx2fc_l2_rcv_work(struct work_struct *work_s)
 {
-   struct fcoe_percpu_s *bg = arg;
+   struct fcoe_percpu_s *bg;
struct sk_buff *skb;
 
-   set_user_nice(current, MIN_NICE);
-   set_current_state(TASK_INTERRUPTIBLE);
-   while (!kthread_should_stop()) {
-   schedule();
-   spin_lock_bh(>fcoe_rx_list.lock);
-   while ((skb = __skb_dequeue(>fcoe_rx_list)) != NULL) {
-   spin_unlock_bh(>fcoe_rx_list.lock);
-   bnx2fc_recv_frame(skb);
-   spin_lock_bh(>fcoe_rx_list.lock);
-   }
-   __set_current_state(TASK_INTERRUPTIBLE);
+   bg = container_of(work_s, struct fcoe_percpu_s, work);
+
+   spin_lock_bh(>fcoe_rx_list.lock);
+   while ((skb = __skb_dequeue(>fcoe_rx_list)) != NULL) {
spin_unlock_bh(>fcoe_rx_list.lock);
+   bnx2fc_recv_frame(skb);
+   spin_lock_bh(>fcoe_rx_list.lock);
}
-   __set_current_state(TASK_RUNNING);
-   return 0;
+   spin_unlock_bh(>fcoe_rx_list.lock);
 }
 
 
@@ -2574,7 +2568,6 @@ static int bnx2fc_slave_configure(struct scsi_device 
*sdev)
 static int __init bnx2fc_mod_init(void)
 {
struct fcoe_percpu_s *bg;
-   struct task_struct *l2_thread;
int rc = 0;
unsigned int cpu = 0;
struct bnx2fc_percpu_s *p;
@@ -2607,17 +2600,7 @@ static int __init bnx2fc_mod_init(void)
 
bg = _global;
skb_queue_head_init(>fcoe_rx_list);
-   l2_thread = kthread_create(bnx2fc_l2_rcv_thread,
-  (void *)bg,
-  "bnx2fc_l2_thread");
-   if (IS_ERR(l2_thread)) {
-   rc = PTR_ERR(l2_thread);
-   goto free_wq;
-   }
-   wake_up_process(l2_thread);
-   spin_lock_bh(>fcoe_rx_list.lock);
-   bg->kthread = l2_thread;
-   spin_unlock_bh(>fcoe_rx_list.lock);
+   INIT_WORK(>work, bnx2fc_l2_rcv_work);
 
for_each_possible_cpu(cpu) {
p = _cpu(bnx2fc_percpu, cpu);
@@ -2630,8 +2613,6 @@ static int __init bnx2fc_mod_init(void)
 
return 0;
 
-free_wq:
-   destroy_workqueue(bnx2fc_wq);
 release_bt:
bnx2fc_release_transport();
 detach_ft:
@@ -2644,9 +2625,6 @@ static void __exit bnx2fc_mod_exit(void)
 {
LIST_HEAD(to_be_deleted);
struct bnx2fc_hba *hba, *next;
-   struct fcoe_percpu_s *bg;
-   struct task_struct *l2_thread;
-   struct sk_buff *skb;
unsigned int cpu = 0;
 
/*
@@ -2676,18 +2654,7 @@ static void __exit bnx2fc_mod_exit(void)
}
cnic_unregister_driver(CNIC_ULP_FCOE);
 
-   /* Destroy global thread */
-   bg = _global;
-   spin_lock_bh(>fcoe_rx_list.lock);
-   l2_thread = bg->kthread;
-   bg->kthread = NULL;
-   while ((skb = __skb_dequeue(>fcoe_rx_list)) != NULL)
-   kfree_skb(skb);
-
-   spin_unlock_bh(>fcoe_rx_list.lock);
-
-   if (l2_thread)
-   kthread_stop(l2_thread);
+   flush_work(_global.work);
 
for_each_possible_cpu(cpu) {
struct bnx2fc_percpu_s *p;
diff --git a/include/scsi/libfcoe.h b/include/scsi/libfcoe.h
index 722d3264d3bf..ca6fb3c0f172 100644
--- a

Re: [PATCH] scsi: bnx2i: convert to kworker

[Sebastian Andrzej Siewior]

On 2016-07-04 19:40:37 [+0200], To linux-scsi@vger.kernel.org wrote:
> The driver creates its own per-CPU threads which are updated based on CPU
> hotplug events. It is also possible to use kworkers and remove some of the
> infrastructure get the same job done while saving a few lines of code.

ping

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] scsi: bnx2fc: convert per-CPU thread to kworker

[Sebastian Andrzej Siewior]

On 2016-07-05 18:08:46 [+0200], To linux-scsi@vger.kernel.org wrote:
> The driver creates its own per-CPU threads which are updated based on CPU
> hotplug events. It is also possible to use kworkers and remove some of the
> infrastructure get the same job done while saving a few lines of code.

ping

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] scsi: bnx2fc: convert per-CPU thread to kworker

[Sebastian Andrzej Siewior]

On 07/20/2016 01:04 PM, Johannes Thumshirn wrote:
> Hi Sebastian,

Hi Johannes,

> I've tried to test your series and unfortunately I encountered the following
> oops.
thanks for testing.
…
> 
> I'll try to narrow it down to the specific patch, but currently I'm a bit
> busy, sorry.

I looked over the patch and noticed nothing. It was possible to run the
worker but somehow the spinlock exploded.

> 
> Johannes
> 
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4 v2] scsi: bnx2fc: convert bnx2fc_l2_rcv_thread() to worker

[Sebastian Andrzej Siewior]

This is not driven by the hotplug conversation but while I am at it
looks like a good candidate. Converting the thread to a kworker user
removes also the kthread member from struct fcoe_percpu_s.

This driver uses the struct fcoe_percpu_s but it does not need the
crc_eof_page member, only the work item and fcoe_rx_list. So it is
removed there.

We had one thread so we only use the kworker on the current CPU. If
someone knows how spread this nicely, it would only require the usage of
schedule_work_on() instead schedule_work() :)

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Cc: fcoe-de...@open-fcoe.org
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
v1…v2: remove __set_current_state(TASK_INTERRUPTIBLE); from
   bnx2fc_l2_rcv_work()

 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 57 +--
 include/scsi/libfcoe.h|  1 -
 2 files changed, 12 insertions(+), 46 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c 
b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index 10e1d42b1cb7..b0d8dd70ab9e 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -479,7 +479,7 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct 
net_device *dev,
 
__skb_queue_tail(>fcoe_rx_list, skb);
if (bg->fcoe_rx_list.qlen == 1)
-   wake_up_process(bg->kthread);
+   schedule_work(>work);
 
spin_unlock(>fcoe_rx_list.lock);
 
@@ -489,26 +489,20 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct 
net_device *dev,
return -1;
 }
 
-static int bnx2fc_l2_rcv_thread(void *arg)
+static void bnx2fc_l2_rcv_work(struct work_struct *work_s)
 {
-   struct fcoe_percpu_s *bg = arg;
+   struct fcoe_percpu_s *bg;
struct sk_buff *skb;
 
-   set_user_nice(current, MIN_NICE);
-   set_current_state(TASK_INTERRUPTIBLE);
-   while (!kthread_should_stop()) {
-   schedule();
-   spin_lock_bh(>fcoe_rx_list.lock);
-   while ((skb = __skb_dequeue(>fcoe_rx_list)) != NULL) {
-   spin_unlock_bh(>fcoe_rx_list.lock);
-   bnx2fc_recv_frame(skb);
-   spin_lock_bh(>fcoe_rx_list.lock);
-   }
-   __set_current_state(TASK_INTERRUPTIBLE);
+   bg = container_of(work_s, struct fcoe_percpu_s, work);
+
+   spin_lock_bh(>fcoe_rx_list.lock);
+   while ((skb = __skb_dequeue(>fcoe_rx_list)) != NULL) {
spin_unlock_bh(>fcoe_rx_list.lock);
+   bnx2fc_recv_frame(skb);
+   spin_lock_bh(>fcoe_rx_list.lock);
}
-   __set_current_state(TASK_RUNNING);
-   return 0;
+   spin_unlock_bh(>fcoe_rx_list.lock);
 }
 
 
@@ -2574,7 +2568,6 @@ static int bnx2fc_slave_configure(struct scsi_device 
*sdev)
 static int __init bnx2fc_mod_init(void)
 {
struct fcoe_percpu_s *bg;
-   struct task_struct *l2_thread;
int rc = 0;
unsigned int cpu = 0;
struct bnx2fc_percpu_s *p;
@@ -2607,17 +2600,7 @@ static int __init bnx2fc_mod_init(void)
 
bg = _global;
skb_queue_head_init(>fcoe_rx_list);
-   l2_thread = kthread_create(bnx2fc_l2_rcv_thread,
-  (void *)bg,
-  "bnx2fc_l2_thread");
-   if (IS_ERR(l2_thread)) {
-   rc = PTR_ERR(l2_thread);
-   goto free_wq;
-   }
-   wake_up_process(l2_thread);
-   spin_lock_bh(>fcoe_rx_list.lock);
-   bg->kthread = l2_thread;
-   spin_unlock_bh(>fcoe_rx_list.lock);
+   INIT_WORK(>work, bnx2fc_l2_rcv_work);
 
for_each_possible_cpu(cpu) {
p = _cpu(bnx2fc_percpu, cpu);
@@ -2630,8 +2613,6 @@ static int __init bnx2fc_mod_init(void)
 
return 0;
 
-free_wq:
-   destroy_workqueue(bnx2fc_wq);
 release_bt:
bnx2fc_release_transport();
 detach_ft:
@@ -2644,9 +2625,6 @@ static void __exit bnx2fc_mod_exit(void)
 {
LIST_HEAD(to_be_deleted);
struct bnx2fc_hba *hba, *next;
-   struct fcoe_percpu_s *bg;
-   struct task_struct *l2_thread;
-   struct sk_buff *skb;
unsigned int cpu = 0;
 
/*
@@ -2676,18 +2654,7 @@ static void __exit bnx2fc_mod_exit(void)
}
cnic_unregister_driver(CNIC_ULP_FCOE);
 
-   /* Destroy global thread */
-   bg = _global;
-   spin_lock_bh(>fcoe_rx_list.lock);
-   l2_thread = bg->kthread;
-   bg->kthread = NULL;
-   while ((skb = __skb_dequeue(>fcoe_rx_list)) != NULL)
-   kfree_skb(skb);
-
-   spin_unlock_bh(>fcoe_rx_list.lock);
-
-   if (l2_thread)
-   kthread_stop(l2_thread);
+   flush_work(_global.work);
 
for_each_possible_cpu(cpu) {
struct bnx2fc_percpu_s *p;
diff --git a/include/scsi

[PATCH 3/4] scsi: bnx2fc: annoate unlock + release for sparse

[Sebastian Andrzej Siewior]

The caller of bnx2fc_abts_cleanup() holds the tgt->tgt_lock lock and it
is expected to release the lock during wait_for_completion() and acquire
the lock afterwards.

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc_io.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_io.c b/drivers/scsi/bnx2fc/bnx2fc_io.c
index f501095f91ac..634e1e539850 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_io.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_io.c
@@ -1080,6 +1080,8 @@ int bnx2fc_eh_device_reset(struct scsi_cmnd *sc_cmd)
 }
 
 static int bnx2fc_abts_cleanup(struct bnx2fc_cmd *io_req)
+__releases(tgt->tgt_lock)
+__acquires(tgt->tgt_lock)
 {
struct bnx2fc_rport *tgt = io_req->tgt;
int rc = SUCCESS;
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] scsi: bnx2fc: convert per-CPU thread to kworker

[Sebastian Andrzej Siewior]

The driver creates its own per-CPU threads which are updated based on CPU
hotplug events. It is also possible to use kworkers and remove some of the
infrastructure get the same job done while saving a few lines of code.

bnx2fc_percpu_io_thread() becomes bnx2fc_percpu_io_work() which is
mostly the same code. The outer loop (kthread_should_stop()) gets
removed and the remaining code is shifted to the left.
In bnx2fc_process_new_cqes() the code checked for ->iothread to
decide if there is an active per-CPU thread. With the kworkers this
is no longer possible nor required. The allocation of a new work item
(via bnx2fc_alloc_work()) does no longer happen with the ->fp_work_lock
lock held. It performs only a memory allocation + initialization which
does not require any kind of serialization. The lock is held while
adding the new member to fps->work_list list.

The remaining part is the removal CPU hotplug notifier since it is taken
care by the kworker code.

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc.h  |   2 +-
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 157 ++
 drivers/scsi/bnx2fc/bnx2fc_hwi.c  |  22 +++---
 3 files changed, 32 insertions(+), 149 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc.h b/drivers/scsi/bnx2fc/bnx2fc.h
index fdd4eb4e41b2..fdd89935cac7 100644
--- a/drivers/scsi/bnx2fc/bnx2fc.h
+++ b/drivers/scsi/bnx2fc/bnx2fc.h
@@ -168,7 +168,7 @@ extern struct fcoe_percpu_s bnx2fc_global;
 extern struct workqueue_struct *bnx2fc_wq;
 
 struct bnx2fc_percpu_s {
-   struct task_struct *iothread;
+   struct work_struct work;
struct list_head work_list;
spinlock_t fp_work_lock;
 };
diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c 
b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index d6800afd0232..ab08d3ab8a44 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -127,13 +127,6 @@ module_param_named(log_fka, bnx2fc_log_fka, uint, 
S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(log_fka, " Print message to kernel log when fcoe is "
"initiating a FIP keep alive when debug logging is enabled.");
 
-static int bnx2fc_cpu_callback(struct notifier_block *nfb,
-unsigned long action, void *hcpu);
-/* notification function for CPU hotplug events */
-static struct notifier_block bnx2fc_cpu_notifier = {
-   .notifier_call = bnx2fc_cpu_callback,
-};
-
 static inline struct net_device *bnx2fc_netdev(const struct fc_lport *lport)
 {
return ((struct bnx2fc_interface *)
@@ -621,39 +614,32 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
 }
 
 /**
- * bnx2fc_percpu_io_thread - thread per cpu for ios
+ * bnx2fc_percpu_io_work - work per cpu for ios
  *
- * @arg:   ptr to bnx2fc_percpu_info structure
+ * @work_s:The work struct
  */
-int bnx2fc_percpu_io_thread(void *arg)
+static void bnx2fc_percpu_io_work(struct work_struct *work_s)
 {
-   struct bnx2fc_percpu_s *p = arg;
+   struct bnx2fc_percpu_s *p;
struct bnx2fc_work *work, *tmp;
LIST_HEAD(work_list);
 
-   set_user_nice(current, MIN_NICE);
-   set_current_state(TASK_INTERRUPTIBLE);
-   while (!kthread_should_stop()) {
-   schedule();
-   spin_lock_bh(>fp_work_lock);
-   while (!list_empty(>work_list)) {
-   list_splice_init(>work_list, _list);
-   spin_unlock_bh(>fp_work_lock);
+   p = container_of(work_s, struct bnx2fc_percpu_s, work);
 
-   list_for_each_entry_safe(work, tmp, _list, list) {
-   list_del_init(>list);
-   bnx2fc_process_cq_compl(work->tgt, work->wqe);
-   kfree(work);
-   }
-
-   spin_lock_bh(>fp_work_lock);
-   }
-   __set_current_state(TASK_INTERRUPTIBLE);
+   spin_lock_bh(>fp_work_lock);
+   while (!list_empty(>work_list)) {
+   list_splice_init(>work_list, _list);
spin_unlock_bh(>fp_work_lock);
-   }
-   __set_current_state(TASK_RUNNING);
 
-   return 0;
+   list_for_each_entry_safe(work, tmp, _list, list) {
+   list_del_init(>list);
+   bnx2fc_process_cq_compl(work->tgt, work->wqe);
+   kfree(work);
+   }
+
+   spin_lock_bh(>fp_work_lock);
+   }
+   spin_unlock_bh(>fp_work_lock);
 }
 
 static struct fc_host_statistics *bnx2fc_get_host_stats(struct Scsi_Host 
*shost)
@@ -2570,91 +2556,6 @@ static struct fcoe_transport bnx2fc_transport = {
.disable = bnx2fc_disable,
 };
 
-/**
- * bnx2fc_percpu_thread_create - 

[PATCH 2/4] scsi: bnx2fc: clean up header definitions

[Sebastian Andrzej Siewior]

- All symbols which are only used within one .c file are marked static
  and removed from the bnx2fc.h file if possible.

- the declarion of bnx2fc_percpu is moved into the header file

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc.h  |  5 +
 drivers/scsi/bnx2fc/bnx2fc_els.c  |  4 ++--
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 14 +++---
 drivers/scsi/bnx2fc/bnx2fc_hwi.c  |  8 +++-
 drivers/scsi/bnx2fc/bnx2fc_io.c   |  2 +-
 5 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc.h b/drivers/scsi/bnx2fc/bnx2fc.h
index fdd89935cac7..64308584405d 100644
--- a/drivers/scsi/bnx2fc/bnx2fc.h
+++ b/drivers/scsi/bnx2fc/bnx2fc.h
@@ -172,6 +172,7 @@ struct bnx2fc_percpu_s {
struct list_head work_list;
spinlock_t fp_work_lock;
 };
+DECLARE_PER_CPU(struct bnx2fc_percpu_s, bnx2fc_percpu);
 
 struct bnx2fc_fw_stats {
u64 fc_crc_cnt;
@@ -513,7 +514,6 @@ void bnx2fc_cmd_mgr_free(struct bnx2fc_cmd_mgr *cmgr);
 void bnx2fc_get_link_state(struct bnx2fc_hba *hba);
 char *bnx2fc_get_next_rqe(struct bnx2fc_rport *tgt, u8 num_items);
 void bnx2fc_return_rqe(struct bnx2fc_rport *tgt, u8 num_items);
-int bnx2fc_get_paged_crc_eof(struct sk_buff *skb, int tlen);
 int bnx2fc_send_rrq(struct bnx2fc_cmd *aborted_io_req);
 int bnx2fc_send_adisc(struct bnx2fc_rport *tgt, struct fc_frame *fp);
 int bnx2fc_send_logo(struct bnx2fc_rport *tgt, struct fc_frame *fp);
@@ -537,7 +537,6 @@ void bnx2fc_init_task(struct bnx2fc_cmd *io_req,
 void bnx2fc_add_2_sq(struct bnx2fc_rport *tgt, u16 xid);
 void bnx2fc_ring_doorbell(struct bnx2fc_rport *tgt);
 int bnx2fc_eh_abort(struct scsi_cmnd *sc_cmd);
-int bnx2fc_eh_host_reset(struct scsi_cmnd *sc_cmd);
 int bnx2fc_eh_target_reset(struct scsi_cmnd *sc_cmd);
 int bnx2fc_eh_device_reset(struct scsi_cmnd *sc_cmd);
 void bnx2fc_rport_event_handler(struct fc_lport *lport,
@@ -570,8 +569,6 @@ struct fc_seq *bnx2fc_elsct_send(struct fc_lport *lport, 
u32 did,
   struct fc_frame *,
   void *),
  void *arg, u32 timeout);
-void bnx2fc_arm_cq(struct bnx2fc_rport *tgt);
-int bnx2fc_process_new_cqes(struct bnx2fc_rport *tgt);
 void bnx2fc_process_cq_compl(struct bnx2fc_rport *tgt, u16 wqe);
 struct bnx2fc_rport *bnx2fc_tgt_lookup(struct fcoe_port *port,
 u32 port_id);
diff --git a/drivers/scsi/bnx2fc/bnx2fc_els.c b/drivers/scsi/bnx2fc/bnx2fc_els.c
index 5beea776b9f5..68ca518d34b0 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_els.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_els.c
@@ -254,7 +254,7 @@ int bnx2fc_send_rls(struct bnx2fc_rport *tgt, struct 
fc_frame *fp)
return rc;
 }
 
-void bnx2fc_srr_compl(struct bnx2fc_els_cb_arg *cb_arg)
+static void bnx2fc_srr_compl(struct bnx2fc_els_cb_arg *cb_arg)
 {
struct bnx2fc_mp_req *mp_req;
struct fc_frame_header *fc_hdr, *fh;
@@ -364,7 +364,7 @@ void bnx2fc_srr_compl(struct bnx2fc_els_cb_arg *cb_arg)
kref_put(_io_req->refcount, bnx2fc_cmd_release);
 }
 
-void bnx2fc_rec_compl(struct bnx2fc_els_cb_arg *cb_arg)
+static void bnx2fc_rec_compl(struct bnx2fc_els_cb_arg *cb_arg)
 {
struct bnx2fc_cmd *orig_io_req, *new_io_req;
struct bnx2fc_cmd *rec_req;
diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c 
b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index ab08d3ab8a44..10e1d42b1cb7 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -49,7 +49,7 @@ struct workqueue_struct *bnx2fc_wq;
  * Here the io threads are per cpu but the l2 thread is just one
  */
 struct fcoe_percpu_s bnx2fc_global;
-DEFINE_SPINLOCK(bnx2fc_global_lock);
+static DEFINE_SPINLOCK(bnx2fc_global_lock);
 
 static struct cnic_ulp_ops bnx2fc_cnic_cb;
 static struct libfc_function_template bnx2fc_libfc_fcn_templ;
@@ -107,22 +107,22 @@ MODULE_PARM_DESC(debug_logging,
"\t\t0x10 - fcoe L2 fame related logs.\n"
"\t\t0xff - LOG all messages.");
 
-uint bnx2fc_devloss_tmo;
+static uint bnx2fc_devloss_tmo;
 module_param_named(devloss_tmo, bnx2fc_devloss_tmo, uint, S_IRUGO);
 MODULE_PARM_DESC(devloss_tmo, " Change devloss_tmo for the remote ports "
"attached via bnx2fc.");
 
-uint bnx2fc_max_luns = BNX2FC_MAX_LUN;
+static uint bnx2fc_max_luns = BNX2FC_MAX_LUN;
 module_param_named(max_luns, bnx2fc_max_luns, uint, S_IRUGO);
 MODULE_PARM_DESC(max_luns, " Change the default max_lun per SCSI host. Default 
"
"0x.");
 
-uint bnx2fc_queue_depth;
+static uint bnx2fc_queue_depth;
 module_param_named(queue_depth, bnx2fc_queue_depth, uint, S_IRUGO);
 MODULE_PARM_DESC(queue_depth, " Change the default queue depth of SCSI 

[PATCH] scsi: bnx2i: convert to kworker

[Sebastian Andrzej Siewior]

The driver creates its own per-CPU threads which are updated based on CPU
hotplug events. It is also possible to use kworkers and remove some of the
infrastructure get the same job done while saving a few lines of code.

The DECLARE_PER_CPU() definition is moved into the header file where it
belongs. bnx2i_percpu_io_thread() becomes bnx2i_percpu_io_work() which is
mostly the same code. The outer loop (kthread_should_stop()) gets removed and
the remaining code is shifted to the left.
bnx2i_queue_scsi_cmd_resp() is mostly the same. The code checked ->iothread to
decide if there is an active per-CPU thread. With the kworkers this is no
longer possible nor required.
The allocation of struct bnx2i_work does not happen with ->p_work_lock held
which is not required. I am unsure about the call-stack so I can't say
if this qualifies it for the allocation with GFP_KERNEL instead of
GFP_ATOMIC (it is not _bh lock but as I said, I don't know the context).
The allocation case has been reversed so the inner if case is called on
!bnx2i_work and is just the invocation one function since the lock is not
held during allocation. The init of the new bnx2i_work struct is now
done also without the ->p_work_lock held: it is a new object, nobody
knows about it yet. It should be enough to hold the lock while adding
this item to the list. I am unsure about that atomic_inc() so I keep
things as they were.

The remaining part is the removal CPU hotplug notifier since it is taken
care by the kworker code.

This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2i/bnx2i.h  |  11 +---
 drivers/scsi/bnx2i/bnx2i_hwi.c  | 101 ++---
 drivers/scsi/bnx2i/bnx2i_init.c | 121 +++-
 3 files changed, 53 insertions(+), 180 deletions(-)

diff --git a/drivers/scsi/bnx2i/bnx2i.h b/drivers/scsi/bnx2i/bnx2i.h
index ed7f3228e234..78cdc493bab5 100644
--- a/drivers/scsi/bnx2i/bnx2i.h
+++ b/drivers/scsi/bnx2i/bnx2i.h
@@ -31,7 +31,6 @@
 #include 
 #include 
 #include 
-#include 
 
 #include 
 #include 
@@ -775,12 +774,11 @@ struct bnx2i_work {
 };
 
 struct bnx2i_percpu_s {
-   struct task_struct *iothread;
+   struct work_struct work;
struct list_head work_list;
spinlock_t p_work_lock;
 };
 
-
 /* Global variables */
 extern unsigned int error_mask1, error_mask2;
 extern u64 iscsi_error_mask;
@@ -797,7 +795,7 @@ extern unsigned int rq_size;
 
 extern struct device_attribute *bnx2i_dev_attributes[];
 
-
+DECLARE_PER_CPU(struct bnx2i_percpu_s, bnx2i_percpu);
 
 /*
  * Function Prototypes
@@ -875,8 +873,5 @@ extern void bnx2i_print_active_cmd_queue(struct bnx2i_conn 
*conn);
 extern void bnx2i_print_xmit_pdu_queue(struct bnx2i_conn *conn);
 extern void bnx2i_print_recv_state(struct bnx2i_conn *conn);
 
-extern int bnx2i_percpu_io_thread(void *arg);
-extern int bnx2i_process_scsi_cmd_resp(struct iscsi_session *session,
-  struct bnx2i_conn *bnx2i_conn,
-  struct cqe *cqe);
+extern void bnx2i_percpu_io_work(struct work_struct *work);
 #endif
diff --git a/drivers/scsi/bnx2i/bnx2i_hwi.c b/drivers/scsi/bnx2i/bnx2i_hwi.c
index 42921dbba927..9be58f6523b3 100644
--- a/drivers/scsi/bnx2i/bnx2i_hwi.c
+++ b/drivers/scsi/bnx2i/bnx2i_hwi.c
@@ -19,8 +19,6 @@
 #include 
 #include "bnx2i.h"
 
-DECLARE_PER_CPU(struct bnx2i_percpu_s, bnx2i_percpu);
-
 /**
  * bnx2i_get_cid_num - get cid from ep
  * @ep:endpoint pointer
@@ -1350,9 +1348,9 @@ int bnx2i_send_fw_iscsi_init_msg(struct bnx2i_hba *hba)
  *
  * process SCSI CMD Response CQE & complete the request to SCSI-ML
  */
-int bnx2i_process_scsi_cmd_resp(struct iscsi_session *session,
-   struct bnx2i_conn *bnx2i_conn,
-   struct cqe *cqe)
+static int bnx2i_process_scsi_cmd_resp(struct iscsi_session *session,
+  struct bnx2i_conn *bnx2i_conn,
+  struct cqe *cqe)
 {
struct iscsi_conn *conn = bnx2i_conn->cls_conn->dd_data;
struct bnx2i_hba *hba = bnx2i_conn->hba;
@@ -1862,45 +1860,37 @@ static void bnx2i_process_cmd_cleanup_resp(struct 
iscsi_session *session,
 
 
 /**
- * bnx2i_percpu_io_thread - thread per cpu for ios
+ * bnx2i_percpu_io_work - thread per cpu for ios
  *
- * @arg:   ptr to bnx2i_percpu_info structure
+ * @work_s:The work struct
  */
-int bnx2i_percpu_io_thread(void *arg)
+void bnx2i_percpu_io_work(struct work_struct *work_s)
 {
-   struct bnx2i_percpu_s *p = arg;
+   struct bnx2i_percpu_s *p;
struct bnx2i_work *work, *tmp;
LIST_HEAD(work_list);
 
-   set_user_nice(current, MIN_NICE);
+   p = container_of(work_s, struct bnx2i_percpu_s, 

Re: [PATCH v2] scsi/fcoe: convert to kworker

[Sebastian Andrzej Siewior]

On 06/10/2016 12:38 PM, Johannes Thumshirn wrote:
…

> 
> Tested in a Boot from FCoE scenario using a BCM57840.

This got merged over the weekend. Thanks for that. I will try to look
into the other two (bnx2i, bnx2fc) and convert them as well within this
week.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PREEMPT-RT] [PATCH v2] scsi/fcoe: convert to kworker

[Sebastian Andrzej Siewior]

On 06/09/2016 03:15 PM, Laurence Oberman wrote:
> Hello

Hi,

> Apologies, somehow this fell off my radar.
> I will get the FCOE test bed up and get it done ASAP.

Thanks

> 
> Regards
> Laurence

Sebastian

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PREEMPT-RT] [PATCH v2] scsi/fcoe: convert to kworker

[Sebastian Andrzej Siewior]

On 04/22/2016 06:39 PM, Laurence Oberman wrote:
> I have fcoe for testing.
> I will pull this in next week and test it.

any update?

> 
> Laurence Oberman
> Principal Software Maintenance Engineer
> Red Hat Global Support Services

Sebastian

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PREEMPT-RT] [PATCH v2] scsi/fcoe: convert to kworker

[Sebastian Andrzej Siewior]

On 04/12/2016 05:16 PM, Sebastian Andrzej Siewior wrote:
> The driver creates its own per-CPU threads which are updated based on
> CPU hotplug events. It is also possible to use kworkers and remove some
> of the kthread infrastrucure.
> 
> The code checked ->thread to decide if there is an active per-CPU
> thread. By using the kworker infrastructure this is no longer possible (or
> required). The thread pointer is saved in `kthread' instead of `thread' so
> anything trying to use thread is caught by the compiler. Currently only the
> bnx2fc driver is using struct fcoe_percpu_s and the kthread member.
> 
> After a CPU went offline, we may still enqueue items on the "offline"
> CPU. This isn't much of a problem. The work will be done on a random
> CPU. The allocated crc_eof_page page won't be cleaned up. It is probably
> expected that the CPU comes up at some point so it should not be a
> problem. The crc_eof_page memory is released of course once the module is
> removed.
> 
> This patch was only compile-tested due to -ENODEV.
> 
> Cc: Vasu Dev <vasu@intel.com>
> Cc: "James E.J. Bottomley" <j...@linux.vnet.ibm.com>
> Cc: "Martin K. Petersen" <martin.peter...@oracle.com>
> Cc: Christoph Hellwig <h...@lst.de>
> Cc: fcoe-de...@open-fcoe.org
> Cc: linux-scsi@vger.kernel.org
> Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
> ---
> v1…v2: use kworker instead of smbthread as per hch
> 
> If you want this I would the same for the two bnx drivers.

*ping*

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] scsi/fcoe: convert to kworker

[Sebastian Andrzej Siewior]

The driver creates its own per-CPU threads which are updated based on
CPU hotplug events. It is also possible to use kworkers and remove some
of the kthread infrastrucure.

The code checked ->thread to decide if there is an active per-CPU
thread. By using the kworker infrastructure this is no longer possible (or
required). The thread pointer is saved in `kthread' instead of `thread' so
anything trying to use thread is caught by the compiler. Currently only the
bnx2fc driver is using struct fcoe_percpu_s and the kthread member.

After a CPU went offline, we may still enqueue items on the "offline"
CPU. This isn't much of a problem. The work will be done on a random
CPU. The allocated crc_eof_page page won't be cleaned up. It is probably
expected that the CPU comes up at some point so it should not be a
problem. The crc_eof_page memory is released of course once the module is
removed.

This patch was only compile-tested due to -ENODEV.

Cc: Vasu Dev <vasu@intel.com>
Cc: "James E.J. Bottomley" <j...@linux.vnet.ibm.com>
Cc: "Martin K. Petersen" <martin.peter...@oracle.com>
Cc: Christoph Hellwig <h...@lst.de>
Cc: fcoe-de...@open-fcoe.org
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
v1…v2: use kworker instead of smbthread as per hch

If you want this I would the same for the two bnx drivers.

 drivers/scsi/bnx2fc/bnx2fc_fcoe.c |   8 +-
 drivers/scsi/fcoe/fcoe.c  | 276 --
 include/scsi/libfcoe.h|   6 +-
 3 files changed, 34 insertions(+), 256 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c 
b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index d7029ea5d319..cfb1b5b40d6c 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -466,7 +466,7 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct 
net_device *dev,
 
__skb_queue_tail(>fcoe_rx_list, skb);
if (bg->fcoe_rx_list.qlen == 1)
-   wake_up_process(bg->thread);
+   wake_up_process(bg->kthread);
 
spin_unlock(>fcoe_rx_list.lock);
 
@@ -2663,7 +2663,7 @@ static int __init bnx2fc_mod_init(void)
}
wake_up_process(l2_thread);
spin_lock_bh(>fcoe_rx_list.lock);
-   bg->thread = l2_thread;
+   bg->kthread = l2_thread;
spin_unlock_bh(>fcoe_rx_list.lock);
 
for_each_possible_cpu(cpu) {
@@ -2736,8 +2736,8 @@ static void __exit bnx2fc_mod_exit(void)
/* Destroy global thread */
bg = _global;
spin_lock_bh(>fcoe_rx_list.lock);
-   l2_thread = bg->thread;
-   bg->thread = NULL;
+   l2_thread = bg->kthread;
+   bg->kthread = NULL;
while ((skb = __skb_dequeue(>fcoe_rx_list)) != NULL)
kfree_skb(skb);
 
diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 0efe7112fc1f..f7c7ccc156da 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -67,9 +67,6 @@ static DEFINE_MUTEX(fcoe_config_mutex);
 
 static struct workqueue_struct *fcoe_wq;
 
-/* fcoe_percpu_clean completion.  Waiter protected by fcoe_create_mutex */
-static DECLARE_COMPLETION(fcoe_flush_completion);
-
 /* fcoe host list */
 /* must only by accessed under the RTNL mutex */
 static LIST_HEAD(fcoe_hostlist);
@@ -80,7 +77,6 @@ static int fcoe_reset(struct Scsi_Host *);
 static int fcoe_xmit(struct fc_lport *, struct fc_frame *);
 static int fcoe_rcv(struct sk_buff *, struct net_device *,
struct packet_type *, struct net_device *);
-static int fcoe_percpu_receive_thread(void *);
 static void fcoe_percpu_clean(struct fc_lport *);
 static int fcoe_link_ok(struct fc_lport *);
 
@@ -107,7 +103,6 @@ static int fcoe_ddp_setup(struct fc_lport *, u16, struct 
scatterlist *,
 static int fcoe_ddp_done(struct fc_lport *, u16);
 static int fcoe_ddp_target(struct fc_lport *, u16, struct scatterlist *,
   unsigned int);
-static int fcoe_cpu_callback(struct notifier_block *, unsigned long, void *);
 static int fcoe_dcb_app_notification(struct notifier_block *notifier,
 ulong event, void *ptr);
 
@@ -136,11 +131,6 @@ static struct notifier_block fcoe_notifier = {
.notifier_call = fcoe_device_notification,
 };
 
-/* notification function for CPU hotplug events */
-static struct notifier_block fcoe_cpu_notifier = {
-   .notifier_call = fcoe_cpu_callback,
-};
-
 /* notification function for DCB events */
 static struct notifier_block dcb_notifier = {
.notifier_call = fcoe_dcb_app_notification,
@@ -1245,152 +1235,21 @@ static int __exit fcoe_if_exit(void)
return 0;
 }
 
-/**
- * fcoe_percpu_thread_create() - Create a receive thread for an online CPU
- * @cpu: The CPU index of the CPU to create a receive thread for
- */
-static void fcoe_percpu_thread_create(unsigned int cpu)
+static void fcoe_t

Re: [PATCH 01/11] scsi/fcoe: lock online CPUs in fcoe_percpu_clean()

[Sebastian Andrzej Siewior]

On 04/08/2016 03:30 PM, Sebastian Andrzej Siewior wrote:
> On 03/15/2016 09:19 AM, Christoph Hellwig wrote:
>> On Fri, Mar 11, 2016 at 05:32:15PM +0100, Sebastian Andrzej Siewior wrote:
>>> alloc_workqueue() in setup and then queue_work_on(cpu, , item)? item
>>> should be struct work_struct but all I have is a skb. Is there an easy
>>> way to get this attached?
>>
>> Good question.  There is skb->cb, but it looks like it doesn't have
>> space for an additional work_item in the fcoe case.  Maybe have
>> a per-cpu work_struct and keep all the list handling as-is for now?
> 
> Okay. Let me try this. What about the few fixes from the series (which
> apply before the rework to smbboot theads)?

okay kworker. This does not look good. I have it converted what I miss
flushing work when CPU goes down and ensuring not to queue work while
the CPU is down.

- cpu_online(x) is racy. In DOWN_PREPARE the worker is deactivated /
  stopped. However slightly later the bit from the CPU mask is removed.

- Whatever is queued and did not make it before the CPU went down seems
  to be delayed until the CPU comes back online.

- if the worker keeps running while the CPU is going down the worker
  continues running on a different CPU.

So I don't see how the former two points can be solved without keeping
track of CPUs in a CPU notifier. Getting pushed to a different CPU be
probably less of an issue if we would have a work-item and would not
need to rely on the per-CPU list.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/11] scsi/fcoe: lock online CPUs in fcoe_percpu_clean()

[Sebastian Andrzej Siewior]

On 03/15/2016 09:19 AM, Christoph Hellwig wrote:
> On Fri, Mar 11, 2016 at 05:32:15PM +0100, Sebastian Andrzej Siewior wrote:
>> alloc_workqueue() in setup and then queue_work_on(cpu, , item)? item
>> should be struct work_struct but all I have is a skb. Is there an easy
>> way to get this attached?
> 
> Good question.  There is skb->cb, but it looks like it doesn't have
> space for an additional work_item in the fcoe case.  Maybe have
> a per-cpu work_struct and keep all the list handling as-is for now?

Okay. Let me try this. What about the few fixes from the series (which
apply before the rework to smbboot theads)?

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] MAINTAINERS: Update the email address of James E.J. Bottomley

[Sebastian Andrzej Siewior]

after sending a few patches to the odin.com email address I noticed that
it is no longer valid.

Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 2a37ee56f4c5..4307201a866c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9668,7 +9668,7 @@ F:drivers/scsi/sg.c
 F: include/scsi/sg.h
 
 SCSI SUBSYSTEM
-M: "James E.J. Bottomley" <jbottom...@odin.com>
+M: "James E.J. Bottomley" <james.bottom...@hansenpartnership.com>
 T: git git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git
 M: "Martin K. Petersen" <martin.peter...@oracle.com>
 T: git git://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git
-- 
2.7.0

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/11] scsi/fcoe: lock online CPUs in fcoe_percpu_clean()

[Sebastian Andrzej Siewior]

On 03/11/2016 05:17 PM, Christoph Hellwig wrote:
> On Fri, Mar 11, 2016 at 04:28:53PM +0100, Sebastian Andrzej Siewior wrote:
>> for_each_possible_cpu() with a cpu_online() + `thread' check possibly does
>> the job. But there is a tiny race: Say CPU5 is reported online but is
>> going down. And after fcoe_percpu_clean() saw that CPU5 is online it
>> decided to enqueue a packet. After dev_alloc_skb() returned a skb
>> that CPU is offline (or say the notifier destroyed the kthread). So we
>> would OOps because `thread' is NULL.
>> An alternative would be to lock the CPUs during our loop (so no CPU is
>> going away) and then we iterate over the online mask.
> 
> I've looked over this and the following patches, and I suspect
> the right thing to do for fcoe and bnx2 is to convert them to use the
> generic workqueue code instead of reinventing it poorly.

alloc_workqueue() in setup and then queue_work_on(cpu, , item)? item
should be struct work_struct but all I have is a skb. Is there an easy
way to get this attached?

Sebastian

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/11] scsi/fcoe: remove CONFIG_SMP in fcoe_percpu_thread_destroy()

[Sebastian Andrzej Siewior]

There is only a marginal win in code size if the !SMP code is ifdeffed
out. The compiler should be able to optimize almost everything out.

Remove the ifdeffery for readability sake.

Cc: Vasu Dev <vasu@intel.com>
Cc: "James E.J. Bottomley" <jbottom...@odin.com>
Cc: "Martin K. Petersen" <martin.peter...@oracle.com>
Cc: Christoph Hellwig <h...@lst.de>
Cc: fcoe-de...@open-fcoe.org
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/fcoe/fcoe.c | 13 -
 1 file changed, 13 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 2b0d207f4b2b..efbc8a1438ef 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -1284,10 +1284,8 @@ static void fcoe_percpu_thread_destroy(unsigned int cpu)
struct task_struct *thread;
struct page *crc_eof;
struct sk_buff *skb;
-#ifdef CONFIG_SMP
struct fcoe_percpu_s *p0;
unsigned targ_cpu = get_cpu();
-#endif /* CONFIG_SMP */
 
FCOE_DBG("Destroying receive thread for CPU %d\n", cpu);
 
@@ -1301,7 +1299,6 @@ static void fcoe_percpu_thread_destroy(unsigned int cpu)
p->crc_eof_offset = 0;
spin_unlock_bh(>fcoe_rx_list.lock);
 
-#ifdef CONFIG_SMP
/*
 * Don't bother moving the skb's if this context is running
 * on the same CPU that is having its thread destroyed. This
@@ -1343,16 +1340,6 @@ static void fcoe_percpu_thread_destroy(unsigned int cpu)
spin_unlock_bh(>fcoe_rx_list.lock);
}
put_cpu();
-#else
-   /*
-* This a non-SMP scenario where the singular Rx thread is
-* being removed. Free all skbs and stop the thread.
-*/
-   spin_lock_bh(>fcoe_rx_list.lock);
-   while ((skb = __skb_dequeue(>fcoe_rx_list)) != NULL)
-   kfree_skb(skb);
-   spin_unlock_bh(>fcoe_rx_list.lock);
-#endif
 
if (thread)
kthread_stop(thread);
-- 
2.7.0

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 09/11] scsi: bnx2i: convert to smpboot thread

[Sebastian Andrzej Siewior]

The driver creates its own per-CPU threads which are updated based on
CPU hotplug events. It is also possible to delegate this task to the
smpboot-thread infrastructure and get the same job done while saving a
few lines of code.
The code checked ->iothread to decide if there is an active per-CPU
thread. With the smpboot infrastructure this is no longer possible and I
replaced its logic with the ->active member. The thread pointer is saved
in `kthread' instead of `iothread' so anything trying to use `thread' is
caught by the compiler.

The remaining part of the conversion is mostly straightforward.
This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: "James E.J. Bottomley" <jbottom...@odin.com>
Cc: "Martin K. Petersen" <martin.peter...@oracle.com>
Cc: Christoph Hellwig <h...@lst.de>
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2i/bnx2i.h  |   4 +-
 drivers/scsi/bnx2i/bnx2i_hwi.c  |  45 +--
 drivers/scsi/bnx2i/bnx2i_init.c | 162 
 3 files changed, 69 insertions(+), 142 deletions(-)

diff --git a/drivers/scsi/bnx2i/bnx2i.h b/drivers/scsi/bnx2i/bnx2i.h
index ed7f3228e234..2bbccf571a18 100644
--- a/drivers/scsi/bnx2i/bnx2i.h
+++ b/drivers/scsi/bnx2i/bnx2i.h
@@ -775,9 +775,10 @@ struct bnx2i_work {
 };
 
 struct bnx2i_percpu_s {
-   struct task_struct *iothread;
+   struct task_struct *kthread;
struct list_head work_list;
spinlock_t p_work_lock;
+   bool active;
 };
 
 
@@ -875,7 +876,6 @@ extern void bnx2i_print_active_cmd_queue(struct bnx2i_conn 
*conn);
 extern void bnx2i_print_xmit_pdu_queue(struct bnx2i_conn *conn);
 extern void bnx2i_print_recv_state(struct bnx2i_conn *conn);
 
-extern int bnx2i_percpu_io_thread(void *arg);
 extern int bnx2i_process_scsi_cmd_resp(struct iscsi_session *session,
   struct bnx2i_conn *bnx2i_conn,
   struct cqe *cqe);
diff --git a/drivers/scsi/bnx2i/bnx2i_hwi.c b/drivers/scsi/bnx2i/bnx2i_hwi.c
index fb072cc5e9fd..ec3969732846 100644
--- a/drivers/scsi/bnx2i/bnx2i_hwi.c
+++ b/drivers/scsi/bnx2i/bnx2i_hwi.c
@@ -1860,47 +1860,6 @@ static void bnx2i_process_cmd_cleanup_resp(struct 
iscsi_session *session,
complete(_conn->cmd_cleanup_cmpl);
 }
 
-
-/**
- * bnx2i_percpu_io_thread - thread per cpu for ios
- *
- * @arg:   ptr to bnx2i_percpu_info structure
- */
-int bnx2i_percpu_io_thread(void *arg)
-{
-   struct bnx2i_percpu_s *p = arg;
-   struct bnx2i_work *work, *tmp;
-   LIST_HEAD(work_list);
-
-   set_user_nice(current, MIN_NICE);
-
-   while (!kthread_should_stop()) {
-   spin_lock_bh(>p_work_lock);
-   while (!list_empty(>work_list)) {
-   list_splice_init(>work_list, _list);
-   spin_unlock_bh(>p_work_lock);
-
-   list_for_each_entry_safe(work, tmp, _list, list) {
-   list_del_init(>list);
-   /* work allocated in the bh, freed here */
-   bnx2i_process_scsi_cmd_resp(work->session,
-   work->bnx2i_conn,
-   >cqe);
-   atomic_dec(>bnx2i_conn->work_cnt);
-   kfree(work);
-   }
-   spin_lock_bh(>p_work_lock);
-   }
-   set_current_state(TASK_INTERRUPTIBLE);
-   spin_unlock_bh(>p_work_lock);
-   schedule();
-   }
-   __set_current_state(TASK_RUNNING);
-
-   return 0;
-}
-
-
 /**
  * bnx2i_queue_scsi_cmd_resp - queue cmd completion to the percpu thread
  * @bnx2i_conn:bnx2i connection
@@ -1941,7 +1900,7 @@ static int bnx2i_queue_scsi_cmd_resp(struct iscsi_session 
*session,
 
p = _cpu(bnx2i_percpu, cpu);
spin_lock(>p_work_lock);
-   if (unlikely(!p->iothread)) {
+   if (unlikely(!p->active)) {
rc = -EINVAL;
goto err;
}
@@ -1954,7 +1913,7 @@ static int bnx2i_queue_scsi_cmd_resp(struct iscsi_session 
*session,
memcpy(_work->cqe, cqe, sizeof(struct cqe));
list_add_tail(_work->list, >work_list);
atomic_inc(_conn->work_cnt);
-   wake_up_process(p->iothread);
+   wake_up_process(p->kthread);
spin_unlock(>p_work_lock);
goto done;
} else
diff --git a/drivers/scsi/bnx2i/bnx2i_init.c b/drivers/scsi/bnx2i/bnx2i_init.c
index c8b410c24cf0..4ec8c552cc95 100644
--- a/drivers/scsi/bnx2i/bnx2i_init.c
+++ b/drivers/scsi/bnx2i/bnx2i_init.c
@@ -13,8 +13,8 @@
  * Previously M

[PATCH 03/11] scsi/fcoe: drop locking in fcoe_percpu_thread_destroy() if cpu == targ_cpu

[Sebastian Andrzej Siewior]

The locking here is not required. At the begin of the function we hold
the lock and assign NULL to p->thread. This is used in other places to
ensure that nobody adds new items to the list.
Also, in the cpu != targ_cpu case we don't hold the lock of p-> as well.
This makes is consistent.

Cc: Vasu Dev <vasu@intel.com>
Cc: "James E.J. Bottomley" <jbottom...@odin.com>
Cc: "Martin K. Petersen" <martin.peter...@oracle.com>
Cc: Christoph Hellwig <h...@lst.de>
Cc: fcoe-de...@open-fcoe.org
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/fcoe/fcoe.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index efbc8a1438ef..50e9e980563e 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -1334,10 +1334,8 @@ static void fcoe_percpu_thread_destroy(unsigned int cpu)
 * will reach this case and we will drop all skbs and later
 * stop the thread.
 */
-   spin_lock_bh(>fcoe_rx_list.lock);
while ((skb = __skb_dequeue(>fcoe_rx_list)) != NULL)
kfree_skb(skb);
-   spin_unlock_bh(>fcoe_rx_list.lock);
}
put_cpu();
 
-- 
2.7.0

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 04/11] scsi/fcoe: rename p0 to p_target in fcoe_percpu_thread_destroy()

[Sebastian Andrzej Siewior]

The `p' and `p0' variables have very small names on can get mixed up
easily. Thus I rename `p0' to `p_target' so it sounds more like the
target pointer than `p0' does.

Cc: Vasu Dev <vasu@intel.com>
Cc: "James E.J. Bottomley" <jbottom...@odin.com>
Cc: "Martin K. Petersen" <martin.peter...@oracle.com>
Cc: Christoph Hellwig <h...@lst.de>
Cc: fcoe-de...@open-fcoe.org
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/fcoe/fcoe.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 50e9e980563e..06f56b7f51c2 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -1284,7 +1284,7 @@ static void fcoe_percpu_thread_destroy(unsigned int cpu)
struct task_struct *thread;
struct page *crc_eof;
struct sk_buff *skb;
-   struct fcoe_percpu_s *p0;
+   struct fcoe_percpu_s *p_target;
unsigned targ_cpu = get_cpu();
 
FCOE_DBG("Destroying receive thread for CPU %d\n", cpu);
@@ -1305,15 +1305,15 @@ static void fcoe_percpu_thread_destroy(unsigned int cpu)
 * can easily happen when the module is removed.
 */
if (cpu != targ_cpu) {
-   p0 = _cpu(fcoe_percpu, targ_cpu);
-   spin_lock_bh(>fcoe_rx_list.lock);
-   if (p0->thread) {
+   p_target = _cpu(fcoe_percpu, targ_cpu);
+   spin_lock_bh(_target->fcoe_rx_list.lock);
+   if (p_target->thread) {
FCOE_DBG("Moving frames from CPU %d to CPU %d\n",
 cpu, targ_cpu);
 
while ((skb = __skb_dequeue(>fcoe_rx_list)) != NULL)
-   __skb_queue_tail(>fcoe_rx_list, skb);
-   spin_unlock_bh(>fcoe_rx_list.lock);
+   __skb_queue_tail(_target->fcoe_rx_list, skb);
+   spin_unlock_bh(_target->fcoe_rx_list.lock);
} else {
/*
 * The targeted CPU is not initialized and cannot accept
@@ -1322,7 +1322,7 @@ static void fcoe_percpu_thread_destroy(unsigned int cpu)
 */
while ((skb = __skb_dequeue(>fcoe_rx_list)) != NULL)
kfree_skb(skb);
-   spin_unlock_bh(>fcoe_rx_list.lock);
+   spin_unlock_bh(_target->fcoe_rx_list.lock);
}
} else {
/*
-- 
2.7.0

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 05/11] scsi/fcoe: drop the p_target lock earlier if there is no thread online

[Sebastian Andrzej Siewior]

If the thread on the target CPU is not online then all skbs are freed.
There is no need to hold p_target's lock during that period.

Cc: Vasu Dev <vasu@intel.com>
Cc: "James E.J. Bottomley" <jbottom...@odin.com>
Cc: "Martin K. Petersen" <martin.peter...@oracle.com>
Cc: Christoph Hellwig <h...@lst.de>
Cc: fcoe-de...@open-fcoe.org
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/fcoe/fcoe.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 06f56b7f51c2..a065b31a7a02 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -1320,9 +1320,10 @@ static void fcoe_percpu_thread_destroy(unsigned int cpu)
 * new  skbs. Unlock the targeted CPU and drop the skbs
 * on the CPU that is going offline.
 */
+   spin_unlock_bh(_target->fcoe_rx_list.lock);
+
while ((skb = __skb_dequeue(>fcoe_rx_list)) != NULL)
kfree_skb(skb);
-   spin_unlock_bh(_target->fcoe_rx_list.lock);
}
} else {
/*
-- 
2.7.0

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 10/11] scsi: bnx2fc: fix hotplug race in bnx2fc_process_new_cqes()

[Sebastian Andrzej Siewior]

The ->iothread is accessed without holding the lock. Take this:

 CPU A  CPU B
------
bnx2fc_process_new_cqes()   bnx2fc_percpu_thread_destroy()
 spin_lock_bh(fp_work_lock);
 fps->iothread != NULL
 list_add_tail(work)
 spin_unlock_bh(>fp_work_lock); spin_lock_bh(>fp_work_lock);
 fps->iothread = NULL
 if (fps->iothread && work)
...
 else
  bnx2fc_process_cq_compl(work)  bnx2fc_process_cq_compl(work);

CPU A will process wqe despite having it added to the work list of CPU
B which will at the same time clean up the queued wqe.

The fix is to add the item to the list and wakeup the thread while still
holding the lock. If the item was not added to the list then the
`process' variable is still true in which case we have to do manually.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: "James E.J. Bottomley" <jbottom...@odin.com>
Cc: "Martin K. Petersen" <martin.peter...@oracle.com>
Cc: Christoph Hellwig <h...@lst.de>
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc_hwi.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
index 28c671b609b2..1427062e86f0 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
@@ -1045,6 +1045,7 @@ int bnx2fc_process_new_cqes(struct bnx2fc_rport *tgt)
struct bnx2fc_work *work = NULL;
struct bnx2fc_percpu_s *fps = NULL;
unsigned int cpu = wqe % num_possible_cpus();
+   bool process = true;
 
fps = _cpu(bnx2fc_percpu, cpu);
spin_lock_bh(>fp_work_lock);
@@ -1052,16 +1053,16 @@ int bnx2fc_process_new_cqes(struct bnx2fc_rport *tgt)
goto unlock;
 
work = bnx2fc_alloc_work(tgt, wqe);
-   if (work)
+   if (work) {
list_add_tail(>list,
  >work_list);
+   wake_up_process(fps->iothread);
+   process = false;
+   }
 unlock:
spin_unlock_bh(>fp_work_lock);
 
-   /* Pending work request completion */
-   if (fps->iothread && work)
-   wake_up_process(fps->iothread);
-   else
+   if (process)
bnx2fc_process_cq_compl(tgt, wqe);
num_free_sqes++;
}
-- 
2.7.0

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 08/11] scsi/fcoe: convert to smpboot thread

[Sebastian Andrzej Siewior]

The driver creates its own per-CPU threads which are updated based on
CPU hotplug events. It is also possible to delegate this task to the
smpboot-thread infrastructure and get the same job done while saving a
few lines of code.

The code checked ->thread to decide if there is an active per-CPU
thread. With the smpboot infrastructure this is no longer possible and I
replaced its logic with the ->active member. The thread pointer is saved
in `kthread' instead of `thread' so anything trying to use thread is
caught by the compiler.

The ->park() callback cleans up the resources if a CPU is going down. At
least one CPU has to be online (and not parked) and the skbs are moved to
this CPU. On module removal the ->cleanup() is invoked instead and all skbs
are purged.

The remaining part of the conversion is mostly straightforward.
This patch was only compile-tested due to -ENODEV.

Cc: Vasu Dev <vasu@intel.com>
Cc: "James E.J. Bottomley" <jbottom...@odin.com>
Cc: "Martin K. Petersen" <martin.peter...@oracle.com>
Cc: Christoph Hellwig <h...@lst.de>
Cc: fcoe-de...@open-fcoe.org
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c |   8 +-
 drivers/scsi/fcoe/fcoe.c  | 281 ++
 include/scsi/libfcoe.h|   6 +-
 3 files changed, 112 insertions(+), 183 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c 
b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index 67405c628864..f5bc11b2e884 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -457,7 +457,7 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct 
net_device *dev,
 
__skb_queue_tail(>fcoe_rx_list, skb);
if (bg->fcoe_rx_list.qlen == 1)
-   wake_up_process(bg->thread);
+   wake_up_process(bg->kthread);
 
spin_unlock(>fcoe_rx_list.lock);
 
@@ -2654,7 +2654,7 @@ static int __init bnx2fc_mod_init(void)
}
wake_up_process(l2_thread);
spin_lock_bh(>fcoe_rx_list.lock);
-   bg->thread = l2_thread;
+   bg->kthread = l2_thread;
spin_unlock_bh(>fcoe_rx_list.lock);
 
for_each_possible_cpu(cpu) {
@@ -2727,8 +2727,8 @@ static void __exit bnx2fc_mod_exit(void)
/* Destroy global thread */
bg = _global;
spin_lock_bh(>fcoe_rx_list.lock);
-   l2_thread = bg->thread;
-   bg->thread = NULL;
+   l2_thread = bg->kthread;
+   bg->kthread = NULL;
while ((skb = __skb_dequeue(>fcoe_rx_list)) != NULL)
kfree_skb(skb);
 
diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 4a877ab95d44..2bc570e96663 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -80,7 +81,6 @@ static int fcoe_reset(struct Scsi_Host *);
 static int fcoe_xmit(struct fc_lport *, struct fc_frame *);
 static int fcoe_rcv(struct sk_buff *, struct net_device *,
struct packet_type *, struct net_device *);
-static int fcoe_percpu_receive_thread(void *);
 static void fcoe_percpu_clean(struct fc_lport *);
 static int fcoe_link_ok(struct fc_lport *);
 
@@ -107,7 +107,6 @@ static int fcoe_ddp_setup(struct fc_lport *, u16, struct 
scatterlist *,
 static int fcoe_ddp_done(struct fc_lport *, u16);
 static int fcoe_ddp_target(struct fc_lport *, u16, struct scatterlist *,
   unsigned int);
-static int fcoe_cpu_callback(struct notifier_block *, unsigned long, void *);
 static int fcoe_dcb_app_notification(struct notifier_block *notifier,
 ulong event, void *ptr);
 
@@ -136,11 +135,6 @@ static struct notifier_block fcoe_notifier = {
.notifier_call = fcoe_device_notification,
 };
 
-/* notification function for CPU hotplug events */
-static struct notifier_block fcoe_cpu_notifier = {
-   .notifier_call = fcoe_cpu_callback,
-};
-
 /* notification function for DCB events */
 static struct notifier_block dcb_notifier = {
.notifier_call = fcoe_dcb_app_notification,
@@ -1245,55 +1239,15 @@ static int __exit fcoe_if_exit(void)
return 0;
 }
 
-/**
- * fcoe_percpu_thread_create() - Create a receive thread for an online CPU
- * @cpu: The CPU index of the CPU to create a receive thread for
- */
-static void fcoe_percpu_thread_create(unsigned int cpu)
+static struct fcoe_percpu_s *fcoe_thread_cleanup_local(unsigned int cpu)
 {
-   struct fcoe_percpu_s *p;
-   struct task_struct *thread;
-
-   p = _cpu(fcoe_percpu, cpu);
-
-   thread = kthread_create_on_node(fcoe_percpu_receive_thread,
-   (void *)p, cpu_to_node(cpu),
-   "fcoethread/%d", cpu);
-
-   if (likely(!IS_ERR(thread)))

[PATCH 07/11] scsi/fcoe: drop the crc_eof page early

[Sebastian Andrzej Siewior]

On cleanup we free the crc_eof_page after all skbs are freed. There is
no reason why it can't be done earlier. We hold a reference to that page
and each skb does.

Cc: Vasu Dev <vasu@intel.com>
Cc: "James E.J. Bottomley" <jbottom...@odin.com>
Cc: "Martin K. Petersen" <martin.peter...@oracle.com>
Cc: Christoph Hellwig <h...@lst.de>
Cc: fcoe-de...@open-fcoe.org
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/fcoe/fcoe.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 15826094cc65..4a877ab95d44 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -1299,6 +1299,8 @@ static void fcoe_percpu_thread_destroy(unsigned int cpu)
p->crc_eof_offset = 0;
spin_unlock_bh(>fcoe_rx_list.lock);
 
+   if (crc_eof)
+   put_page(crc_eof);
/*
 * Don't bother moving the skb's if this context is running
 * on the same CPU that is having its thread destroyed. This
@@ -1342,9 +1344,6 @@ static void fcoe_percpu_thread_destroy(unsigned int cpu)
 
if (thread)
kthread_stop(thread);
-
-   if (crc_eof)
-   put_page(crc_eof);
 }
 
 /**
-- 
2.7.0

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 00/11] SCSI smpboot thread conversion

[Sebastian Andrzej Siewior]

This series converts fcoe, bnx2i and bnx2fc to smpboot thread instead
of their own magic. The fcoe driver ended in more patches than I wanted
but that way it is easier to find the individual code blocks which were
used in the final patch.

The overall diffstat:
  8 files changed, 253 insertions(+), 478 deletions(-)

Sebastian

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 06/11] scsi/fcoe: use skb_queue_splice_tail() intead of manual job

[Sebastian Andrzej Siewior]

skb_queue_splice_tail() is able to do the same thing as a loop with
__skb_dequeue() and __skb_queue_tail() like we have it now.

Cc: Vasu Dev <vasu@intel.com>
Cc: "James E.J. Bottomley" <jbottom...@odin.com>
Cc: "Martin K. Petersen" <martin.peter...@oracle.com>
Cc: Christoph Hellwig <h...@lst.de>
Cc: fcoe-de...@open-fcoe.org
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/fcoe/fcoe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index a065b31a7a02..15826094cc65 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -1311,8 +1311,8 @@ static void fcoe_percpu_thread_destroy(unsigned int cpu)
FCOE_DBG("Moving frames from CPU %d to CPU %d\n",
 cpu, targ_cpu);
 
-   while ((skb = __skb_dequeue(>fcoe_rx_list)) != NULL)
-   __skb_queue_tail(_target->fcoe_rx_list, skb);
+   skb_queue_splice_tail(>fcoe_rx_list,
+ _target->fcoe_rx_list);
spin_unlock_bh(_target->fcoe_rx_list.lock);
} else {
/*
-- 
2.7.0

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 11/11] scsi: bnx2fc: convert to smpboot thread

[Sebastian Andrzej Siewior]

The driver creates its own per-CPU threads which are updated based on
CPU hotplug events. It is also possible to delegate this task to the
smpboot-thread infrastructure and get the same job done while saving a
few lines of code.
The code checked ->iothread to decide if there is an active per-CPU
thread. With the smpboot infrastructure this is no longer possible and I
replaced its logic with the ->active member. The thread pointer is saved
in `kthread' instead of `iothread' so anything trying to use thread is
caught by the compiler.

The remaining part of the conversion is mostly straightforward.
This patch was only compile-tested due to -ENODEV.

Cc: qlogic-storage-upstr...@qlogic.com
Cc: "James E.J. Bottomley" <jbottom...@odin.com>
Cc: "Martin K. Petersen" <martin.peter...@oracle.com>
Cc: Christoph Hellwig <h...@lst.de>
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc.h  |   3 +-
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 187 --
 drivers/scsi/bnx2fc/bnx2fc_hwi.c  |   4 +-
 3 files changed, 64 insertions(+), 130 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc.h b/drivers/scsi/bnx2fc/bnx2fc.h
index 499e369eabf0..7bc5692bb493 100644
--- a/drivers/scsi/bnx2fc/bnx2fc.h
+++ b/drivers/scsi/bnx2fc/bnx2fc.h
@@ -168,9 +168,10 @@ extern struct fcoe_percpu_s bnx2fc_global;
 extern struct workqueue_struct *bnx2fc_wq;
 
 struct bnx2fc_percpu_s {
-   struct task_struct *iothread;
+   struct task_struct *kthread;
struct list_head work_list;
spinlock_t fp_work_lock;
+   bool active;
 };
 
 struct bnx2fc_fw_stats {
diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c 
b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index f5bc11b2e884..12ee035e9c4c 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -14,6 +14,7 @@
  */
 
 #include "bnx2fc.h"
+#include 
 
 static struct list_head adapter_list;
 static struct list_head if_list;
@@ -98,13 +99,6 @@ static void __exit bnx2fc_mod_exit(void);
 unsigned int bnx2fc_debug_level;
 module_param_named(debug_logging, bnx2fc_debug_level, int, S_IRUGO|S_IWUSR);
 
-static int bnx2fc_cpu_callback(struct notifier_block *nfb,
-unsigned long action, void *hcpu);
-/* notification function for CPU hotplug events */
-static struct notifier_block bnx2fc_cpu_notifier = {
-   .notifier_call = bnx2fc_cpu_callback,
-};
-
 static inline struct net_device *bnx2fc_netdev(const struct fc_lport *lport)
 {
return ((struct bnx2fc_interface *)
@@ -591,40 +585,26 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
fc_exch_recv(lport, fp);
 }
 
-/**
- * bnx2fc_percpu_io_thread - thread per cpu for ios
- *
- * @arg:   ptr to bnx2fc_percpu_info structure
- */
-int bnx2fc_percpu_io_thread(void *arg)
+static void bnx2fc_thread_io_process(unsigned int cpu)
 {
-   struct bnx2fc_percpu_s *p = arg;
+   struct bnx2fc_percpu_s *p = per_cpu_ptr(_percpu, cpu);
struct bnx2fc_work *work, *tmp;
LIST_HEAD(work_list);
 
-   set_user_nice(current, MIN_NICE);
-   set_current_state(TASK_INTERRUPTIBLE);
-   while (!kthread_should_stop()) {
-   schedule();
-   spin_lock_bh(>fp_work_lock);
-   while (!list_empty(>work_list)) {
-   list_splice_init(>work_list, _list);
-   spin_unlock_bh(>fp_work_lock);
-
-   list_for_each_entry_safe(work, tmp, _list, list) {
-   list_del_init(>list);
-   bnx2fc_process_cq_compl(work->tgt, work->wqe);
-   kfree(work);
-   }
-
-   spin_lock_bh(>fp_work_lock);
-   }
-   __set_current_state(TASK_INTERRUPTIBLE);
+   spin_lock_bh(>fp_work_lock);
+   while (!list_empty(>work_list)) {
+   list_splice_init(>work_list, _list);
spin_unlock_bh(>fp_work_lock);
-   }
-   __set_current_state(TASK_RUNNING);
 
-   return 0;
+   list_for_each_entry_safe(work, tmp, _list, list) {
+   list_del_init(>list);
+   bnx2fc_process_cq_compl(work->tgt, work->wqe);
+   kfree(work);
+   }
+
+   spin_lock_bh(>fp_work_lock);
+   }
+   spin_unlock_bh(>fp_work_lock);
 }
 
 static struct fc_host_statistics *bnx2fc_get_host_stats(struct Scsi_Host 
*shost)
@@ -2518,34 +2498,9 @@ static struct fcoe_transport bnx2fc_transport = {
.disable = bnx2fc_disable,
 };
 
-/**
- * bnx2fc_percpu_thread_create - Create a receive thread for an
- *  online CPU
- *
- * @cpu: cpu index for the online cpu
- */
-static void bnx2fc_percpu_thread_create(unsigned int cpu)
+st

[PATCH 01/11] scsi/fcoe: lock online CPUs in fcoe_percpu_clean()

[Sebastian Andrzej Siewior]

for_each_possible_cpu() with a cpu_online() + `thread' check possibly does
the job. But there is a tiny race: Say CPU5 is reported online but is
going down. And after fcoe_percpu_clean() saw that CPU5 is online it
decided to enqueue a packet. After dev_alloc_skb() returned a skb
that CPU is offline (or say the notifier destroyed the kthread). So we
would OOps because `thread' is NULL.
An alternative would be to lock the CPUs during our loop (so no CPU is
going away) and then we iterate over the online mask.

Cc: Vasu Dev <vasu@intel.com>
Cc: "James E.J. Bottomley" <jbottom...@odin.com>
Cc: "Martin K. Petersen" <martin.peter...@oracle.com>
Cc: Christoph Hellwig <h...@lst.de>
Cc: fcoe-de...@open-fcoe.org
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
 drivers/scsi/fcoe/fcoe.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 0efe7112fc1f..2b0d207f4b2b 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -2461,12 +2461,10 @@ static void fcoe_percpu_clean(struct fc_lport *lport)
struct sk_buff *skb;
unsigned int cpu;
 
-   for_each_possible_cpu(cpu) {
+   get_online_cpus();
+   for_each_online_cpu(cpu) {
pp = _cpu(fcoe_percpu, cpu);
 
-   if (!pp->thread || !cpu_online(cpu))
-   continue;
-
skb = dev_alloc_skb(0);
if (!skb)
continue;
@@ -2481,6 +2479,7 @@ static void fcoe_percpu_clean(struct fc_lport *lport)
 
wait_for_completion(_flush_completion);
}
+   put_online_cpus();
 }
 
 /**
-- 
2.7.0

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hpsa: fix warning with smp_processor_id() in preemptible

[Sebastian Andrzej Siewior]

* John Kacur | 2013-07-26 16:42:30 [+0200]:

Signed-off-by: John Kacur jka...@redhat.com
Acked-by: Stephen scame...@beardog.cce.hp.com

ping.

I checked the branches for-next, scsi-fixes, fixes and misc at [0] and I
didn't see it. I'm going to take this for 3.10-rt but please don't lose
it on its way to Linus :)

[0] git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git

Sebastian
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] scsi/aacraid: user upper/lower_address() macro to avoid right shift count = width of type warning.

[Sebastian Andrzej Siewior]

Avoids the following warning:
|drivers/scsi/aacraid/src.c: In function 'aac_src_deliver_message':
|drivers/scsi/aacraid/src.c:410:3: warning: right shift count = width of type 
[enabled by default]
|BUG_ON((u32)(address  32) != 0L);
|^
|drivers/scsi/aacraid/src.c:434:2: warning: right shift count = width of type 
[enabled by default]
|   src_writel(dev, MUnit.IQ_H, (address  32)  0x);

Signed-off-by: Sebastian Andrzej Siewior sebast...@breakpoint.cc
---
 drivers/scsi/aacraid/src.c |6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/aacraid/src.c b/drivers/scsi/aacraid/src.c
index 3b021ec..4f6388e 100644
--- a/drivers/scsi/aacraid/src.c
+++ b/drivers/scsi/aacraid/src.c
@@ -407,7 +407,7 @@ static int aac_src_deliver_message(struct fib *fib)
fib-hw_fib_va-header.StructType = FIB_MAGIC2;
fib-hw_fib_va-header.SenderFibAddress = (u32)address;
fib-hw_fib_va-header.u.TimeStamp = 0;
-   BUG_ON((u32)(address  32) != 0L);
+   BUG_ON((upper_32_bits(address) != 0L));
address |= fibsize;
} else {
/* Calculate the amount to the fibsize bits */
@@ -431,8 +431,8 @@ static int aac_src_deliver_message(struct fib *fib)
address |= fibsize;
}
 
-   src_writel(dev, MUnit.IQ_H, (address  32)  0x);
-   src_writel(dev, MUnit.IQ_L, address  0x);
+   src_writel(dev, MUnit.IQ_H, upper_32_bits(address));
+   src_writel(dev, MUnit.IQ_L, lower_32_bits(address));
 
return 0;
 }
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Try #2: Use SCSI read/write(16) with 2TB drives

[Sebastian Andrzej Siewior]

On Wed, Nov 14, 2012 at 12:55:13AM -0500, Jason J. Herne wrote:
 --- a/drivers/scsi/sd.c
 +++ b/drivers/scsi/sd.c
 @@ -887,7 +887,7 @@ static int sd_prep_fn(struct request_queue *q, struct 
 request *rq)
   SCpnt-cmnd[29] = (unsigned char) (this_count  16)  0xff;
   SCpnt-cmnd[30] = (unsigned char) (this_count  8)  0xff;
   SCpnt-cmnd[31] = (unsigned char) this_count  0xff;
 - } else if (block  0x) {
 + } else if (sdp-use_16_for_rw) {
   SCpnt-cmnd[0] += READ_16 - READ_6;
   SCpnt-cmnd[1] = protect | ((rq-cmd_flags  REQ_FUA) ? 0x8 : 
 0);
   SCpnt-cmnd[2] = sizeof(block)  4 ? (unsigned char) (block  
 56)  0xff : 0;
 @@ -2054,6 +2054,9 @@ got_data:
   }
   }
  
 + /* Use read/write(16) for  2TB disks */
 + sdp-use_16_for_rw = (sdkp-capacity  0x);

The comment is pointless if you assume the reading is able to read C. What
would help is a link either here or in patch's description to the mail thread
where it has been descovered that some 2TB devices don't recognize the small
command.

 +
   /* Rescale capacity to 512-byte units */
   if (sector_size == 4096)
   sdkp-capacity = 3;

Sebastian
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] target: Allow for target_submit_cmd() returning errors

[Sebastian Andrzej Siewior]

On 07/18/2012 02:05 AM, Nicholas A. Bellinger wrote:

index 02ace18..9ddf315 100644
--- a/drivers/usb/gadget/tcm_usb_gadget.c
+++ b/drivers/usb/gadget/tcm_usb_gadget.c
@@ -1060,7 +1060,10 @@ static void usbg_cmd_work(struct work_struct *work)
tpg = cmd-fu-tpg;
tv_nexus = tpg-tpg_nexus;
dir = get_cmd_dir(cmd-cmd_buf);
-   if (dir  0) {
+   if (dir  0 ||
+   target_submit_cmd(se_cmd, tv_nexus-tvn_se_sess,
+ cmd-cmd_buf, cmd-sense_iu.sense, 
cmd-unpacked_lun,
+ 0, cmd-prio_attr, dir, TARGET_SCF_UNKNOWN_SIZE)) 
{
transport_init_se_cmd(se_cmd,
tv_nexus-tvn_se_sess-se_tpg-se_tpg_tfo,
tv_nexus-tvn_se_sess, cmd-data_len, DMA_NONE,
@@ -1069,12 +1072,7 @@ static void usbg_cmd_work(struct work_struct *work)
transport_send_check_condition_and_sense(se_cmd,
TCM_UNSUPPORTED_SCSI_OPCODE, 1);
usbg_cleanup_cmd(cmd);
-   return;
}
-
-   target_submit_cmd(se_cmd, tv_nexus-tvn_se_sess,
-   cmd-cmd_buf, cmd-sense_iu.sense, cmd-unpacked_lun,
-   0, cmd-prio_attr, dir, TARGET_SCF_UNKNOWN_SIZE);
  }

  static int usbg_submit_command(struct f_uas *fu,


Looking at this part again target_submit_cmd() has been moved ahead of
target_init_se_cmd() in the exception path, which ends up getting called
twice during a target_get_sess_cmd() failure..

It looks like usbg_cmd_work() + bot_cmd_work() will need some work if
they intends to use a non zero failure to signal exception here, without
invoking a CHECK_CONDITION and sense.  Actually, I'm not even sure
sending a CHECK_CONDITION here after the target_submit_cmd() is going to
work for other fabrics drivers, but it appears to work with usb-gadget.
(Sebastian..?)


For UAS the status/ sense URB is sent right away (without any data) and
this worked the last time I tested.
For BOT things are a little complicated. What I do is to send empty
data (to fill the data pipe) and send a bad status so the other side
learns that the transfer failed somehow. The sense information is lost.
What I should do is to queue this sense data for the next MODE_SENSE
request which will be coming soon. Right now WinXP repeats a failed
command thrice if MODE_SENSE returns no error. This is on my to-fix
list…


So for the moment, lets fix the double se_cmd init and make things a
little easier to read..   Sebastian, please have a look and double check
that the (dir  0) + target_submit_cmd() failures cases are both going
to work as expected whilst sending a CHECK_CONDITION response.


The sense code should only be sent once and
transport_send_check_condition_and_sense() sets
SCF_SENT_CHECK_CONDITION and checks for it so doing it twice should not
do any harm right? But grep does not say when this flag gets removed.



Thanks!

--nab


Sebastian
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html