| Message ID | 20190125072351.3504-9-hmadhani@marvell.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 97a93cea887376e13ea7d473ab349181850f4e39 |
| Headers | show |
| Series | qla2xxx: Driver updates and bug fixes | expand |
On Thu, 2019-01-24 at 23:23 -0800, Himanshu Madhani wrote: > From: Giridhar Malavali <gmalavali@marvell.com> > > This patch fixes SRB allocation flag from GFP_KERNEL to GFP_ATOMIC, to > prevent sleeping in IRQ context > > Signed-off-by: Giridhar Malavali <gmalavali@marvell.com> > Signed-off-by: Himanshu Madhani <hmadhani@marvell.com> > --- > drivers/scsi/qla2xxx/qla_init.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c > index aa72e8316533..3bb4fa97e40a 100644 > --- a/drivers/scsi/qla2xxx/qla_init.c > +++ b/drivers/scsi/qla2xxx/qla_init.c > @@ -1829,7 +1829,7 @@ qla24xx_async_abort_cmd(srb_t *cmd_sp, bool wait) > int rval = QLA_FUNCTION_FAILED; > > sp = qla2xxx_get_qpair_sp(cmd_sp->vha, cmd_sp->qpair, cmd_sp->fcport, > - GFP_KERNEL); > + GFP_ATOMIC); > if (!sp) > goto done; Is this change necessary because this function can be called from inside a timer callback function? Is that callback function qla2x00_sp_timeout()? If so, have you considered to modify that function such that it schedules the work it has to do? Would that be sufficient to avoid that GFP_KERNEL has to be changed into GFP_ATOMIC in qla24xx_async_abort_cmd()? Thanks, Bart.
On 1/25/19, 7:55 AM, "linux-scsi-owner@vger.kernel.org on behalf of Bart Van Assche" <linux-scsi-owner@vger.kernel.org on behalf of bvanassche@acm.org> wrote:
External Email
On Thu, 2019-01-24 at 23:23 -0800, Himanshu Madhani wrote:
> From: Giridhar Malavali <gmalavali@marvell.com>
>
> This patch fixes SRB allocation flag from GFP_KERNEL to GFP_ATOMIC, to
> prevent sleeping in IRQ context
>
> Signed-off-by: Giridhar Malavali <gmalavali@marvell.com>
> Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
> ---
> drivers/scsi/qla2xxx/qla_init.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index aa72e8316533..3bb4fa97e40a 100644
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c
> @@ -1829,7 +1829,7 @@ qla24xx_async_abort_cmd(srb_t *cmd_sp, bool wait)
> int rval = QLA_FUNCTION_FAILED;
>
> sp = qla2xxx_get_qpair_sp(cmd_sp->vha, cmd_sp->qpair, cmd_sp->fcport,
> - GFP_KERNEL);
> + GFP_ATOMIC);
> if (!sp)
> goto done;
Is this change necessary because this function can be called from inside a
timer callback function? Is that callback function qla2x00_sp_timeout()?
If so, have you considered to modify that function such that it schedules
the work it has to do? Would that be sufficient to avoid that GFP_KERNEL
has to be changed into GFP_ATOMIC in qla24xx_async_abort_cmd()?
yes, qla2x00_sp_timeout() is the function. Considering that this happens very rare I did not consider scheduling this further.
Thanks,
Bart.
Hi Bart,
On 1/29/19, 2:30 PM, "Giridhar Malavali" <gmalavali@marvell.com> wrote:
On 1/25/19, 7:55 AM, "linux-scsi-owner@vger.kernel.org on behalf of Bart Van Assche" <linux-scsi-owner@vger.kernel.org on behalf of bvanassche@acm.org> wrote:
External Email
On Thu, 2019-01-24 at 23:23 -0800, Himanshu Madhani wrote:
> From: Giridhar Malavali <gmalavali@marvell.com>
>
> This patch fixes SRB allocation flag from GFP_KERNEL to GFP_ATOMIC, to
> prevent sleeping in IRQ context
>
> Signed-off-by: Giridhar Malavali <gmalavali@marvell.com>
> Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
> ---
> drivers/scsi/qla2xxx/qla_init.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index aa72e8316533..3bb4fa97e40a 100644
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c
> @@ -1829,7 +1829,7 @@ qla24xx_async_abort_cmd(srb_t *cmd_sp, bool wait)
> int rval = QLA_FUNCTION_FAILED;
>
> sp = qla2xxx_get_qpair_sp(cmd_sp->vha, cmd_sp->qpair, cmd_sp->fcport,
> - GFP_KERNEL);
> + GFP_ATOMIC);
> if (!sp)
> goto done;
Is this change necessary because this function can be called from inside a
timer callback function? Is that callback function qla2x00_sp_timeout()?
If so, have you considered to modify that function such that it schedules
the work it has to do? Would that be sufficient to avoid that GFP_KERNEL
has to be changed into GFP_ATOMIC in qla24xx_async_abort_cmd()?
yes, qla2x00_sp_timeout() is the function. Considering that this happens very rare I did not consider scheduling this further.
Thanks,
Bart.
Let us know if there are any other comments?
Thanks,
Himanshu
On 2/4/19 12:22 PM, Himanshu Madhani wrote:
> Let us know if there are any other comments?
Hi Himanshu,
Sorry but I do not have the time to review all patches in this series in
detail. I commented on this patch because I think it's unfortunate to
use GFP_ATOMIC when it's possible to avoid the use of GFP_ATOMIC.
Bart.
On 2/4/19, 6:19 PM, "Bart Van Assche" <bvanassche@acm.org> wrote:
External Email
----------------------------------------------------------------------
On 2/4/19 12:22 PM, Himanshu Madhani wrote:
> Let us know if there are any other comments?
Hi Himanshu,
Sorry but I do not have the time to review all patches in this series in
detail. I commented on this patch because I think it's unfortunate to
use GFP_ATOMIC when it's possible to avoid the use of GFP_ATOMIC.
We will check this further, but right now can we go ahead with this since this could crash.
-- Giri
Bart.
diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c index aa72e8316533..3bb4fa97e40a 100644 --- a/drivers/scsi/qla2xxx/qla_init.c +++ b/drivers/scsi/qla2xxx/qla_init.c @@ -1829,7 +1829,7 @@ qla24xx_async_abort_cmd(srb_t *cmd_sp, bool wait) int rval = QLA_FUNCTION_FAILED; sp = qla2xxx_get_qpair_sp(cmd_sp->vha, cmd_sp->qpair, cmd_sp->fcport, - GFP_KERNEL); + GFP_ATOMIC); if (!sp) goto done;