| Message ID | 20200130114220.23538-2-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dmaengine: Cleanups for symlink handling and debugfs support | expand |
Hi Peter, On Thu, Jan 30, 2020 at 12:41 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > No need to use goto to jump over the > return chan ? chan : ERR_PTR(-EPROBE_DEFER); > We can just revert the check and return right there. > > Do not fail the channel request if the chan->name allocation fails, but > print a warning about it. > > Change the dev_err to dev_warn if sysfs_create_link() fails as it is not > fatal. > > Only attempt to remove the DMA_SLAVE_NAME symlink if it is created - or it > was attempted to be created. > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> Thanks for your patch! > --- a/drivers/dma/dmaengine.c > +++ b/drivers/dma/dmaengine.c > @@ -756,22 +756,24 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name) > } > mutex_unlock(&dma_list_mutex); > > - if (!IS_ERR_OR_NULL(chan)) > - goto found; > - > - return chan ? chan : ERR_PTR(-EPROBE_DEFER); > + if (IS_ERR_OR_NULL(chan)) > + return chan ? chan : ERR_PTR(-EPROBE_DEFER); > > found: > - chan->slave = dev; > chan->name = kasprintf(GFP_KERNEL, "dma:%s", name); > - if (!chan->name) > - return ERR_PTR(-ENOMEM); > + if (!chan->name) { > + dev_warn(dev, > + "Cannot allocate memory for slave symlink name\n"); No need to print a message, as the memory allocator core will have screamed already. > + return chan; > + } > + chan->slave = dev; > > if (sysfs_create_link(&chan->dev->device.kobj, &dev->kobj, > DMA_SLAVE_NAME)) > - dev_err(dev, "Cannot create DMA %s symlink\n", DMA_SLAVE_NAME); > + dev_warn(dev, "Cannot create DMA %s symlink\n", DMA_SLAVE_NAME); > if (sysfs_create_link(&dev->kobj, &chan->dev->device.kobj, chan->name)) > - dev_err(dev, "Cannot create DMA %s symlink\n", chan->name); > + dev_warn(dev, "Cannot create DMA %s symlink\n", chan->name); > + > return chan; > } > EXPORT_SYMBOL_GPL(dma_request_chan); With the above fixed: Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
Hi Geert, On 30/01/2020 17.20, Geert Uytterhoeven wrote: > Hi Peter, > > On Thu, Jan 30, 2020 at 12:41 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: >> No need to use goto to jump over the >> return chan ? chan : ERR_PTR(-EPROBE_DEFER); >> We can just revert the check and return right there. >> >> Do not fail the channel request if the chan->name allocation fails, but >> print a warning about it. >> >> Change the dev_err to dev_warn if sysfs_create_link() fails as it is not >> fatal. >> >> Only attempt to remove the DMA_SLAVE_NAME symlink if it is created - or it >> was attempted to be created. >> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > > Thanks for your patch! > >> --- a/drivers/dma/dmaengine.c >> +++ b/drivers/dma/dmaengine.c >> @@ -756,22 +756,24 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name) >> } >> mutex_unlock(&dma_list_mutex); >> >> - if (!IS_ERR_OR_NULL(chan)) >> - goto found; >> - >> - return chan ? chan : ERR_PTR(-EPROBE_DEFER); >> + if (IS_ERR_OR_NULL(chan)) >> + return chan ? chan : ERR_PTR(-EPROBE_DEFER); >> >> found: >> - chan->slave = dev; >> chan->name = kasprintf(GFP_KERNEL, "dma:%s", name); >> - if (!chan->name) >> - return ERR_PTR(-ENOMEM); >> + if (!chan->name) { >> + dev_warn(dev, >> + "Cannot allocate memory for slave symlink name\n"); > > No need to print a message, as the memory allocator core will have > screamed already. Right, I tend to forget this ;) > >> + return chan; >> + } >> + chan->slave = dev; >> >> if (sysfs_create_link(&chan->dev->device.kobj, &dev->kobj, >> DMA_SLAVE_NAME)) >> - dev_err(dev, "Cannot create DMA %s symlink\n", DMA_SLAVE_NAME); >> + dev_warn(dev, "Cannot create DMA %s symlink\n", DMA_SLAVE_NAME); >> if (sysfs_create_link(&dev->kobj, &chan->dev->device.kobj, chan->name)) >> - dev_err(dev, "Cannot create DMA %s symlink\n", chan->name); >> + dev_warn(dev, "Cannot create DMA %s symlink\n", chan->name); >> + >> return chan; >> } >> EXPORT_SYMBOL_GPL(dma_request_chan); > > With the above fixed: > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Thanks, - Péter > > Gr{oetje,eeting}s, > > Geert > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c index 7b1cefc3213a..75516f9fbab4 100644 --- a/drivers/dma/dmaengine.c +++ b/drivers/dma/dmaengine.c @@ -756,22 +756,24 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name) } mutex_unlock(&dma_list_mutex); - if (!IS_ERR_OR_NULL(chan)) - goto found; - - return chan ? chan : ERR_PTR(-EPROBE_DEFER); + if (IS_ERR_OR_NULL(chan)) + return chan ? chan : ERR_PTR(-EPROBE_DEFER); found: - chan->slave = dev; chan->name = kasprintf(GFP_KERNEL, "dma:%s", name); - if (!chan->name) - return ERR_PTR(-ENOMEM); + if (!chan->name) { + dev_warn(dev, + "Cannot allocate memory for slave symlink name\n"); + return chan; + } + chan->slave = dev; if (sysfs_create_link(&chan->dev->device.kobj, &dev->kobj, DMA_SLAVE_NAME)) - dev_err(dev, "Cannot create DMA %s symlink\n", DMA_SLAVE_NAME); + dev_warn(dev, "Cannot create DMA %s symlink\n", DMA_SLAVE_NAME); if (sysfs_create_link(&dev->kobj, &chan->dev->device.kobj, chan->name)) - dev_err(dev, "Cannot create DMA %s symlink\n", chan->name); + dev_warn(dev, "Cannot create DMA %s symlink\n", chan->name); + return chan; } EXPORT_SYMBOL_GPL(dma_request_chan); @@ -830,13 +832,14 @@ void dma_release_channel(struct dma_chan *chan) /* drop PRIVATE cap enabled by __dma_request_channel() */ if (--chan->device->privatecnt == 0) dma_cap_clear(DMA_PRIVATE, chan->device->cap_mask); + if (chan->slave) { + sysfs_remove_link(&chan->dev->device.kobj, DMA_SLAVE_NAME); sysfs_remove_link(&chan->slave->kobj, chan->name); kfree(chan->name); chan->name = NULL; chan->slave = NULL; } - sysfs_remove_link(&chan->dev->device.kobj, DMA_SLAVE_NAME); mutex_unlock(&dma_list_mutex); } EXPORT_SYMBOL_GPL(dma_release_channel);
No need to use goto to jump over the return chan ? chan : ERR_PTR(-EPROBE_DEFER); We can just revert the check and return right there. Do not fail the channel request if the chan->name allocation fails, but print a warning about it. Change the dev_err to dev_warn if sysfs_create_link() fails as it is not fatal. Only attempt to remove the DMA_SLAVE_NAME symlink if it is created - or it was attempted to be created. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> --- drivers/dma/dmaengine.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-)