Patchwork [5/5] dmaengine: bcm2835: Remove dead code

login
register
mail settings
Submitter Lukas Wunner
Date Dec. 22, 2018, 7:28 a.m.
Message ID <72817c247506d84dd7b2955303a27274c72b5ef5.1545462045.git.lukas@wunner.de>
Download mbox | patch
Permalink /patch/688949/
State New
Headers show

Comments

Lukas Wunner - Dec. 22, 2018, 7:28 a.m.
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(-)
Stefan Wahren - Dec. 28, 2018, 1:26 p.m.
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
Lukas Wunner - Dec. 28, 2018, 1:55 p.m.
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
Stefan Wahren - Dec. 28, 2018, 3:27 p.m.
> 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
Vinod Koul - Jan. 7, 2019, 8:28 a.m.
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
Lukas Wunner - Jan. 8, 2019, 2:18 p.m.
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
Vinod Koul - Jan. 8, 2019, 5:09 p.m.
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.

Patch

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);