Re: [bug report] spi: spi-qcom-qspi: Add DMA mode support

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

On Fri, Jun 16, 2023 at 07:42:30PM +0530, Vijaya Krishna Nivarthi wrote:
> Hello Dan,
> 
> 
> On 6/16/2023 6:12 PM, Dan Carpenter wrote:
> > Hello Vijaya Krishna Nivarthi,
> > 
> > The patch b5762d95607e: "spi: spi-qcom-qspi: Add DMA mode support"
> > from Apr 24, 2023, leads to the following Smatch static checker
> > warning:
> > 
> > 	drivers/spi/spi-qcom-qspi.c:368 qcom_qspi_setup_dma_desc()
> > 	warn: sleeping in atomic context
> > 
> > drivers/spi/spi-qcom-qspi.c
> >      336 static int qcom_qspi_setup_dma_desc(struct qcom_qspi *ctrl,
> >      337                                 struct spi_transfer *xfer)
> >      338 {
> >      339         int ret;
> >      340         struct sg_table *sgt;
> >      341         dma_addr_t dma_ptr_sg;
> >      342         unsigned int dma_len_sg;
> >      343         int i;
> >      344
> >      345         if (ctrl->n_cmd_desc) {
> >      346                 dev_err(ctrl->dev, "Remnant dma buffers n_cmd_desc-%d\n", ctrl->n_cmd_desc);
> >      347                 return -EIO;
> >      348         }
> >      349
> >      350         sgt = (ctrl->xfer.dir == QSPI_READ) ? &xfer->rx_sg : &xfer->tx_sg;
> >      351         if (!sgt->nents || sgt->nents > QSPI_MAX_SG) {
> >      352                 dev_warn_once(ctrl->dev, "Cannot handle %d entries in scatter list\n", sgt->nents);
> >      353                 return -EAGAIN;
> >      354         }
> >      355
> >      356         for (i = 0; i < sgt->nents; i++) {
> >      357                 dma_ptr_sg = sg_dma_address(sgt->sgl + i);
> >      358                 if (!IS_ALIGNED(dma_ptr_sg, QSPI_ALIGN_REQ)) {
> >      359                         dev_warn_once(ctrl->dev, "dma_address not aligned to %d\n", QSPI_ALIGN_REQ);
> >      360                         return -EAGAIN;
> >      361                 }
> >      362         }
> >      363
> >      364         for (i = 0; i < sgt->nents; i++) {
> >      365                 dma_ptr_sg = sg_dma_address(sgt->sgl + i);
> >      366                 dma_len_sg = sg_dma_len(sgt->sgl + i);
> >      367
> > --> 368                 ret = qcom_qspi_alloc_desc(ctrl, dma_ptr_sg, dma_len_sg);
> > 
> > The qcom_qspi_alloc_desc() is a sleeping allocation, but the caller
> > qcom_qspi_transfer_one() is holding spin_lock_irqsave(&ctrl->lock, flags).
> 
> 
> I think We should be able to move qcom_qspi_setup_dma_desc() , and thus
> qcom_qspi_alloc_desc(), out of critical section.
> 
> Right now we are looking into some issues with DMA mode transfer that have
> showed up recently and will likely have a Fixes: patch next week.
> 
> Can we include this change in same series?
> 
> Another option is to include flags __GFP_ATOMIC in call to dma_pool_alloc()
> and upload a Fixes: patch right away.
> 
> Would that work?
> 
> Can you please advise?

This is from static analysis not testing.  It doesn't affect me at all,
so I don't have any opinion.  I imagine that fixing it next week is
fine.

regards,
dan carpenter