| Message ID | 20230710154932.68377-6-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | spi: Header and core clean up and refactoring | expand |
On Mon, Jul 10, 2023 at 06:49:22PM +0300, Andy Shevchenko wrote: > Seems by unknown reason, probably some kind of mis-rebase, > the commit 0c79378c0199 ("spi: add ancillary device support") > adds a dozen of duplicating lines of code. Drop them. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/spi/spi.c | 11 ----------- > 1 file changed, 11 deletions(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index c99ee4164f11..46cbda383228 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -712,17 +712,6 @@ EXPORT_SYMBOL_GPL(spi_add_device); > static int spi_add_device_locked(struct spi_device *spi) > { > struct spi_controller *ctlr = spi->controller; > - struct device *dev = ctlr->dev.parent; > - > - /* Chipselects are numbered 0..max; validate. */ > - if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) { > - dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0), > - ctlr->num_chipselect); > - return -EINVAL; > - } > - > - /* Set the bus ID string */ > - spi_dev_set_name(spi); I see that this is duplicating spi_add_device() (and we really could do better with code sharing there I think) but I can't immediately see where the duplication that's intended to be elimiated is here - where else in the one call path that spi_add_device_locked() has would we do the above? Based on the changelog I was expecting to see some duplicated code in the function itself.
On Mon, Jul 10, 2023 at 06:16:22PM +0100, Mark Brown wrote: > On Mon, Jul 10, 2023 at 06:49:22PM +0300, Andy Shevchenko wrote: > > Seems by unknown reason, probably some kind of mis-rebase, > > the commit 0c79378c0199 ("spi: add ancillary device support") > > adds a dozen of duplicating lines of code. Drop them. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > drivers/spi/spi.c | 11 ----------- > > 1 file changed, 11 deletions(-) > > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > > index c99ee4164f11..46cbda383228 100644 > > --- a/drivers/spi/spi.c > > +++ b/drivers/spi/spi.c > > @@ -712,17 +712,6 @@ EXPORT_SYMBOL_GPL(spi_add_device); > > static int spi_add_device_locked(struct spi_device *spi) > > { > > struct spi_controller *ctlr = spi->controller; > > - struct device *dev = ctlr->dev.parent; > > - > > - /* Chipselects are numbered 0..max; validate. */ > > - if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) { > > - dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0), > > - ctlr->num_chipselect); > > - return -EINVAL; > > - } > > - > > - /* Set the bus ID string */ > > - spi_dev_set_name(spi); > > I see that this is duplicating spi_add_device() (and we really could do > better with code sharing there I think) but I can't immediately see > where the duplication that's intended to be elimiated is here - where > else in the one call path that spi_add_device_locked() has would we do > the above? Based on the changelog I was expecting to see some > duplicated code in the function itself. Oh, by some reason Sebastian wasn't in this rather long Cc list. Added him. Reading again I don't see any useful explanation why that piece of code has to be duplicated among these two functions. It's 100% a copy. Sebastian, can you shed some light here?
Hi, On Tue, Jul 11, 2023 at 02:06:20PM +0300, Andy Shevchenko wrote: > On Mon, Jul 10, 2023 at 06:16:22PM +0100, Mark Brown wrote: > > On Mon, Jul 10, 2023 at 06:49:22PM +0300, Andy Shevchenko wrote: > > > Seems by unknown reason, probably some kind of mis-rebase, > > > the commit 0c79378c0199 ("spi: add ancillary device support") > > > adds a dozen of duplicating lines of code. Drop them. > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > --- > > > drivers/spi/spi.c | 11 ----------- > > > 1 file changed, 11 deletions(-) > > > > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > > > index c99ee4164f11..46cbda383228 100644 > > > --- a/drivers/spi/spi.c > > > +++ b/drivers/spi/spi.c > > > @@ -712,17 +712,6 @@ EXPORT_SYMBOL_GPL(spi_add_device); > > > static int spi_add_device_locked(struct spi_device *spi) > > > { > > > struct spi_controller *ctlr = spi->controller; > > > - struct device *dev = ctlr->dev.parent; > > > - > > > - /* Chipselects are numbered 0..max; validate. */ > > > - if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) { > > > - dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0), > > > - ctlr->num_chipselect); > > > - return -EINVAL; > > > - } > > > - > > > - /* Set the bus ID string */ > > > - spi_dev_set_name(spi); > > > > I see that this is duplicating spi_add_device() (and we really could do > > better with code sharing there I think) but I can't immediately see > > where the duplication that's intended to be elimiated is here - where > > else in the one call path that spi_add_device_locked() has would we do > > the above? Based on the changelog I was expecting to see some > > duplicated code in the function itself. > > Oh, by some reason Sebastian wasn't in this rather long Cc list. > Added him. > > Reading again I don't see any useful explanation why that piece of code has to > be duplicated among these two functions. It's 100% a copy. > > Sebastian, can you shed some light here? The patch in this thread is obviously wrong. It results in the checks never beeing called for spi_add_device_locked(). The copy is in spi_add_device() and those two are not calling into each other. But it should be fine to move the code to the start of __spi_add_device(), which allows removing the duplication. In that case the code will be run with the add_lock held, which is probably what I was worried about two years ago. Looking at it again, the lock is held anyways in case of spi_add_device_locked(). Greetings, -- Sebastian
On Tue, Jul 11, 2023 at 02:01:33PM +0200, Sebastian Reichel wrote: > On Tue, Jul 11, 2023 at 02:06:20PM +0300, Andy Shevchenko wrote: > > On Mon, Jul 10, 2023 at 06:16:22PM +0100, Mark Brown wrote: > > > On Mon, Jul 10, 2023 at 06:49:22PM +0300, Andy Shevchenko wrote: ... > > > > - struct device *dev = ctlr->dev.parent; > > > > - > > > > - /* Chipselects are numbered 0..max; validate. */ > > > > - if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) { > > > > - dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0), > > > > - ctlr->num_chipselect); > > > > - return -EINVAL; > > > > - } > > > > - > > > > - /* Set the bus ID string */ > > > > - spi_dev_set_name(spi); > > > > > > I see that this is duplicating spi_add_device() (and we really could do > > > better with code sharing there I think) but I can't immediately see > > > where the duplication that's intended to be elimiated is here - where > > > else in the one call path that spi_add_device_locked() has would we do > > > the above? Based on the changelog I was expecting to see some > > > duplicated code in the function itself. > > > > Oh, by some reason Sebastian wasn't in this rather long Cc list. > > Added him. > > > > Reading again I don't see any useful explanation why that piece of code has to > > be duplicated among these two functions. It's 100% a copy. > > > > Sebastian, can you shed some light here? > > The patch in this thread is obviously wrong. It results in the > checks never beeing called for spi_add_device_locked(). The copy is > in spi_add_device() and those two are not calling into each other. Ah, now I see, I missed __ in the name. Thank you for opening my eyes! > But it should be fine to move the code to the start of > __spi_add_device(), which allows removing the duplication. In that > case the code will be run with the add_lock held, which is probably > what I was worried about two years ago. Looking at it again, the > lock is held anyways in case of spi_add_device_locked(). Right, I will re-do that.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index c99ee4164f11..46cbda383228 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -712,17 +712,6 @@ EXPORT_SYMBOL_GPL(spi_add_device); static int spi_add_device_locked(struct spi_device *spi) { struct spi_controller *ctlr = spi->controller; - struct device *dev = ctlr->dev.parent; - - /* Chipselects are numbered 0..max; validate. */ - if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) { - dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0), - ctlr->num_chipselect); - return -EINVAL; - } - - /* Set the bus ID string */ - spi_dev_set_name(spi); WARN_ON(!mutex_is_locked(&ctlr->add_lock)); return __spi_add_device(spi);
Seems by unknown reason, probably some kind of mis-rebase, the commit 0c79378c0199 ("spi: add ancillary device support") adds a dozen of duplicating lines of code. Drop them. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/spi/spi.c | 11 ----------- 1 file changed, 11 deletions(-)