| Message ID | 72817c247506d84dd7b2955303a27274c72b5ef5.1545462045.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Raspberry Pi DMA fixes + cleanups | expand |
Hi Lukas, > Lukas Wunner <lukas@wunner.de> hat am 22. Dezember 2018 um 08:28 geschrieben: > > > The BCM2835 DMA driver deletes a channel from a list upon termination > without having added it to a list first. Moreover that operation is > protected by a spinlock which isn't taken anywhere else. These appear > to be remnants of an older version of the driver which accidentally > got mainlined. Remove the dead code. > > While at it remove an outdated comment claiming the driver only supports > cyclic transactions. The driver has been supporting other transaction > types for more than two years. > the fact that your mixing two different changes in one patch results in a very general subject. Please split this up and give them more specific subject lines. Thanks Stefan
On Fri, Dec 28, 2018 at 02:26:00PM +0100, Stefan Wahren wrote: > > Lukas Wunner <lukas@wunner.de> hat am 22. Dezember 2018 um 08:28 geschrieben: > > The BCM2835 DMA driver deletes a channel from a list upon termination > > without having added it to a list first. Moreover that operation is > > protected by a spinlock which isn't taken anywhere else. These appear > > to be remnants of an older version of the driver which accidentally > > got mainlined. Remove the dead code. > > > > While at it remove an outdated comment claiming the driver only supports > > cyclic transactions. The driver has been supporting other transaction > > types for more than two years. > > the fact that your mixing two different changes in one patch results > in a very general subject. In so far as a code comment can be considered code, removal of an obsolete code comment can be referred to as removal of dead code. So the subject seems pertinent to everything contained in this patch from my point of view. > Please split this up and give them more specific subject lines. Frankly I don't consider removal of a 2 line code comment worthy a commit of it's own, so if this is indeed a concern, I'd rather drop it from the patch and leave the obsolete code comment in the file for removal at some other date. Thanks, Lukas
> Lukas Wunner <lukas@wunner.de> hat am 28. Dezember 2018 um 14:55 geschrieben: > > > On Fri, Dec 28, 2018 at 02:26:00PM +0100, Stefan Wahren wrote: > > > Lukas Wunner <lukas@wunner.de> hat am 22. Dezember 2018 um 08:28 geschrieben: > > > The BCM2835 DMA driver deletes a channel from a list upon termination > > > without having added it to a list first. Moreover that operation is > > > protected by a spinlock which isn't taken anywhere else. These appear > > > to be remnants of an older version of the driver which accidentally > > > got mainlined. Remove the dead code. > > > > > > While at it remove an outdated comment claiming the driver only supports > > > cyclic transactions. The driver has been supporting other transaction > > > types for more than two years. > > > > the fact that your mixing two different changes in one patch results > > in a very general subject. > > In so far as a code comment can be considered code, removal of an > obsolete code comment can be referred to as removal of dead code. > So the subject seems pertinent to everything contained in this > patch from my point of view. This wasn't the point. It is common in a driver to remove dead code. So in case other commiters came to the idea to name their changes "remove dead code" it is very hard to distinguish those changes. > > > > Please split this up and give them more specific subject lines. > > Frankly I don't consider removal of a 2 line code comment worthy a > commit of it's own, so if this is indeed a concern, I'd rather drop > it from the patch and leave the obsolete code comment in the file > for removal at some other date. Okay > > Thanks, > > Lukas
On 22-12-18, 08:28, Lukas Wunner wrote: > The BCM2835 DMA driver deletes a channel from a list upon termination > without having added it to a list first. Moreover that operation is > protected by a spinlock which isn't taken anywhere else. These appear > to be remnants of an older version of the driver which accidentally > got mainlined. Remove the dead code. > > While at it remove an outdated comment claiming the driver only supports Ditto, whenever you use also, while at it... think if this is a candidate for splitting up. A patch should do one thing only.. > cyclic transactions. The driver has been supporting other transaction > types for more than two years. > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Cc: Frank Pavlic <f.pavlic@kunbus.de> > Cc: Martin Sperl <kernel@martin.sperl.org> > Cc: Florian Meier <florian.meier@koalo.de> > --- > drivers/dma/bcm2835-dma.c | 13 ------------- > 1 file changed, 13 deletions(-) > > diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c > index e424e232a3d0..633be2ee7f33 100644 > --- a/drivers/dma/bcm2835-dma.c > +++ b/drivers/dma/bcm2835-dma.c > @@ -2,9 +2,6 @@ > /* > * BCM2835 DMA engine support > * > - * This driver only supports cyclic DMA transfers > - * as needed for the I2S module. > - * > * Author: Florian Meier <florian.meier@koalo.de> > * Copyright 2013 > * > @@ -42,7 +39,6 @@ > > struct bcm2835_dmadev { > struct dma_device ddev; > - spinlock_t lock; > void __iomem *base; > struct device_dma_parameters dma_parms; > }; > @@ -64,7 +60,6 @@ struct bcm2835_cb_entry { > > struct bcm2835_chan { > struct virt_dma_chan vc; > - struct list_head node; > > struct dma_slave_config cfg; > unsigned int dreq; > @@ -772,17 +767,11 @@ static int bcm2835_dma_slave_config(struct dma_chan *chan, > static int bcm2835_dma_terminate_all(struct dma_chan *chan) > { > struct bcm2835_chan *c = to_bcm2835_dma_chan(chan); > - struct bcm2835_dmadev *d = to_bcm2835_dma_dev(c->vc.chan.device); > unsigned long flags; > LIST_HEAD(head); > > spin_lock_irqsave(&c->vc.lock, flags); > > - /* Prevent this channel being scheduled */ > - spin_lock(&d->lock); > - list_del_init(&c->node); > - spin_unlock(&d->lock); > - > /* stop DMA activity */ > if (c->desc) { > vchan_terminate_vdesc(&c->desc->vd); > @@ -815,7 +804,6 @@ static int bcm2835_dma_chan_init(struct bcm2835_dmadev *d, int chan_id, > > c->vc.desc_free = bcm2835_dma_desc_free; > vchan_init(&c->vc, &d->ddev); > - INIT_LIST_HEAD(&c->node); > > c->chan_base = BCM2835_DMA_CHANIO(d->base, chan_id); > c->ch = chan_id; > @@ -918,7 +906,6 @@ static int bcm2835_dma_probe(struct platform_device *pdev) > od->ddev.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST; > od->ddev.dev = &pdev->dev; > INIT_LIST_HEAD(&od->ddev.channels); > - spin_lock_init(&od->lock); > > platform_set_drvdata(pdev, od); > > -- > 2.19.2
On Mon, Jan 07, 2019 at 01:58:33PM +0530, Vinod Koul wrote: > On 22-12-18, 08:28, Lukas Wunner wrote: > > The BCM2835 DMA driver deletes a channel from a list upon termination > > without having added it to a list first. Moreover that operation is > > protected by a spinlock which isn't taken anywhere else. These appear > > to be remnants of an older version of the driver which accidentally > > got mainlined. Remove the dead code. > > > > While at it remove an outdated comment claiming the driver only supports > > Ditto, whenever you use also, while at it... think if this is a > candidate for splitting up. > > A patch should do one thing only.. If a set of changes could provoke a regression, separating each into an individual commit makes sense to ease bisecting. However separating trivial and obviously correct cleanups into individual commits actually makes bisecting *harder* because it increases the number of steps to find the offending commit. Thus, clustering related cleanups together makes sense. It is never a good idea to follow rules such as "a patch should do one thing only" slavishly. Nevertheless, happy to oblige if that's what it takes to get the patches in. Thanks, Lukas
On 08-01-19, 15:18, Lukas Wunner wrote: > On Mon, Jan 07, 2019 at 01:58:33PM +0530, Vinod Koul wrote: > > On 22-12-18, 08:28, Lukas Wunner wrote: > > > The BCM2835 DMA driver deletes a channel from a list upon termination > > > without having added it to a list first. Moreover that operation is > > > protected by a spinlock which isn't taken anywhere else. These appear > > > to be remnants of an older version of the driver which accidentally > > > got mainlined. Remove the dead code. > > > > > > While at it remove an outdated comment claiming the driver only supports > > > > Ditto, whenever you use also, while at it... think if this is a > > candidate for splitting up. > > > > A patch should do one thing only.. > > If a set of changes could provoke a regression, separating each into > an individual commit makes sense to ease bisecting. > > However separating trivial and obviously correct cleanups into individual > commits actually makes bisecting *harder* because it increases the number > of steps to find the offending commit. Thus, clustering related cleanups > together makes sense. I disagree, even if it is cleanup I would prefer a patch doing one thing. While working down the line it helps to follow the changes atomically rather than deal with a big pile! > It is never a good idea to follow rules such as "a patch should do one > thing only" slavishly. Agreed on that, but I feel I have a decent reason for this request! > Nevertheless, happy to oblige if that's what it takes to get the patches in.
diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c index e424e232a3d0..633be2ee7f33 100644 --- a/drivers/dma/bcm2835-dma.c +++ b/drivers/dma/bcm2835-dma.c @@ -2,9 +2,6 @@ /* * BCM2835 DMA engine support * - * This driver only supports cyclic DMA transfers - * as needed for the I2S module. - * * Author: Florian Meier <florian.meier@koalo.de> * Copyright 2013 * @@ -42,7 +39,6 @@ struct bcm2835_dmadev { struct dma_device ddev; - spinlock_t lock; void __iomem *base; struct device_dma_parameters dma_parms; }; @@ -64,7 +60,6 @@ struct bcm2835_cb_entry { struct bcm2835_chan { struct virt_dma_chan vc; - struct list_head node; struct dma_slave_config cfg; unsigned int dreq; @@ -772,17 +767,11 @@ static int bcm2835_dma_slave_config(struct dma_chan *chan, static int bcm2835_dma_terminate_all(struct dma_chan *chan) { struct bcm2835_chan *c = to_bcm2835_dma_chan(chan); - struct bcm2835_dmadev *d = to_bcm2835_dma_dev(c->vc.chan.device); unsigned long flags; LIST_HEAD(head); spin_lock_irqsave(&c->vc.lock, flags); - /* Prevent this channel being scheduled */ - spin_lock(&d->lock); - list_del_init(&c->node); - spin_unlock(&d->lock); - /* stop DMA activity */ if (c->desc) { vchan_terminate_vdesc(&c->desc->vd); @@ -815,7 +804,6 @@ static int bcm2835_dma_chan_init(struct bcm2835_dmadev *d, int chan_id, c->vc.desc_free = bcm2835_dma_desc_free; vchan_init(&c->vc, &d->ddev); - INIT_LIST_HEAD(&c->node); c->chan_base = BCM2835_DMA_CHANIO(d->base, chan_id); c->ch = chan_id; @@ -918,7 +906,6 @@ static int bcm2835_dma_probe(struct platform_device *pdev) od->ddev.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST; od->ddev.dev = &pdev->dev; INIT_LIST_HEAD(&od->ddev.channels); - spin_lock_init(&od->lock); platform_set_drvdata(pdev, od);
The BCM2835 DMA driver deletes a channel from a list upon termination without having added it to a list first. Moreover that operation is protected by a spinlock which isn't taken anywhere else. These appear to be remnants of an older version of the driver which accidentally got mainlined. Remove the dead code. While at it remove an outdated comment claiming the driver only supports cyclic transactions. The driver has been supporting other transaction types for more than two years. Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: Frank Pavlic <f.pavlic@kunbus.de> Cc: Martin Sperl <kernel@martin.sperl.org> Cc: Florian Meier <florian.meier@koalo.de> --- drivers/dma/bcm2835-dma.c | 13 ------------- 1 file changed, 13 deletions(-)