re: aha1542: rework locking

re: aha1542: rework locking

[Dan Carpenter]

Hello Ondrej Zary,

The patch 1b0224b0ec60: "aha1542: rework locking" from Feb 6, 2015,
leads to the following static checker warning:

	drivers/scsi/aha1542.c:448 aha1542_queuecommand()
	error: scheduling with locks held: 'spin_lock:host_lock'

drivers/scsi/aha1542.c
   400          /* Use the outgoing mailboxes in a round-robin fashion, because this
   401             is how the host adapter will scan for them */
   402  
   403          spin_lock_irqsave(sh->host_lock, flags);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Holding lock.

   404          mbo = aha1542->aha1542_last_mbo_used + 1;
   405          if (mbo >= AHA1542_MAILBOXES)
   406                  mbo = 0;
   407  

[ snip ]

   442          if (bufflen) {
   443                  struct scatterlist *sg;
   444                  struct chain *cptr;
   445                  int i, sg_count = scsi_sg_count(cmd);
   446  
   447                  ccb[mbo].op = 2;        /* SCSI Initiator Command  w/scatter-gather */
   448                  cmd->host_scribble = kmalloc(sizeof(*cptr)*sg_count,
   449                                                           GFP_KERNEL | GFP_DMA);
                                                                 ^^^^^^^^^^
Sleeping allocation.

   450                  cptr = (struct chain *) cmd->host_scribble;
   451                  if (cptr == NULL) {
   452                          /* free the claimed mailbox slot */
   453                          aha1542->int_cmds[mbo] = NULL;
   454                          spin_unlock_irqrestore(sh->host_lock, flags);
   455                          return SCSI_MLQUEUE_HOST_BUSY;
   456                  }
   457                  scsi_for_each_sg(cmd, sg, sg_count, i) {
   458                          any2scsi(cptr[i].dataptr, isa_page_to_bus(sg_page(sg))
   459                                                                  + sg->offset);
   460                          any2scsi(cptr[i].datalen, sg->length);
   461                  };

regards,
dan carpenter

Re: aha1542: rework locking

[Ondrej Zary]

On Tuesday 21 April 2015, Dan Carpenter wrote:
> Hello Ondrej Zary,
> 
> The patch 1b0224b0ec60: "aha1542: rework locking" from Feb 6, 2015,
> leads to the following static checker warning:
> 
> 	drivers/scsi/aha1542.c:448 aha1542_queuecommand()
> 	error: scheduling with locks held: 'spin_lock:host_lock'
> 
> drivers/scsi/aha1542.c
>    400          /* Use the outgoing mailboxes in a round-robin fashion, because this
>    401             is how the host adapter will scan for them */
>    402  
>    403          spin_lock_irqsave(sh->host_lock, flags);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Holding lock.
> 
>    404          mbo = aha1542->aha1542_last_mbo_used + 1;
>    405          if (mbo >= AHA1542_MAILBOXES)
>    406                  mbo = 0;
>    407  
> 
> [ snip ]
> 
>    442          if (bufflen) {
>    443                  struct scatterlist *sg;
>    444                  struct chain *cptr;
>    445                  int i, sg_count = scsi_sg_count(cmd);
>    446  
>    447                  ccb[mbo].op = 2;        /* SCSI Initiator Command  w/scatter-gather */
>    448                  cmd->host_scribble = kmalloc(sizeof(*cptr)*sg_count,
>    449                                                           GFP_KERNEL | GFP_DMA);
>                                                                  ^^^^^^^^^^
> Sleeping allocation.

Thanks, I missed that as it was already wrong before (since at least when
DEF_SCSI_QCMD was introduced). And it worked despite that.

Should I just change that to GFP_ATOMIC?

-- 
Ondrej Zary

Re: aha1542: rework locking

[Dan Carpenter]

On Tue, Apr 21, 2015 at 12:39:40PM +0200, Ondrej Zary wrote:
> Thanks, I missed that as it was already wrong before (since at least when
> DEF_SCSI_QCMD was introduced). And it worked despite that.

If you turn on CONFIG_DEBUG_ATOMIC_SLEEP=y it will generate a warning
splat.  Otherwise you probably have to get very unlucky for this to
deadlock.

> 
> Should I just change that to GFP_ATOMIC?

The other option is to do the allocation before taking the lock.  That
looks do-able.  You are the expert here by virtue of you touched to code
last.  So you get to decide.  :)

regards,
dan carpenter