[PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor

[PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor

[Andy Shevchenko]

There is no need to use atomic bit operations when allocating a minor
number since it's done under a mutex. Moreover, one of the operations
that is in use is non-atomic anyway.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spidev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index 53a551714265..daeaa4a30290 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -795,7 +795,7 @@ static int spidev_probe(struct spi_device *spi)
 		status = -ENODEV;
 	}
 	if (status == 0) {
-		set_bit(minor, minors);
+		__set_bit(minor, minors);
 		list_add(&spidev->device_entry, &device_list);
 	}
 	mutex_unlock(&device_list_lock);
@@ -823,7 +823,7 @@ static void spidev_remove(struct spi_device *spi)
 
 	list_del(&spidev->device_entry);
 	device_destroy(spidev_class, spidev->devt);
-	clear_bit(MINOR(spidev->devt), minors);
+	__clear_bit(MINOR(spidev->devt), minors);
 	if (spidev->users == 0)
 		kfree(spidev);
 	mutex_unlock(&device_list_lock);
-- 
2.35.1

[PATCH v1 2/4] spidev: Convert BUILD_BUG_ON() to static_assert()

[Andy Shevchenko]

static_assert() is a preferred method to fail build when the certain
constraints are not met. Convert BUILD_BUG_ON() to static_assert().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spidev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index daeaa4a30290..125da8d0e719 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -46,6 +46,7 @@
 
 static DECLARE_BITMAP(minors, N_SPI_MINORS);
 
+static_assert(N_SPI_MINORS > 0 && N_SPI_MINORS <= 256);
 
 /* Bit masks for spi_device.mode management.  Note that incorrect
  * settings for some settings can cause *lots* of trouble for other
@@ -856,7 +857,6 @@ static int __init spidev_init(void)
 	 * that will key udev/mdev to add/remove /dev nodes.  Last, register
 	 * the driver which manages those device numbers.
 	 */
-	BUILD_BUG_ON(N_SPI_MINORS > 256);
 	status = register_chrdev(SPIDEV_MAJOR, "spi", &spidev_fops);
 	if (status < 0)
 		return status;
-- 
2.35.1

[PATCH v1 3/4] spidev: Replace ACPI specific code by device_get_match_data()

[Andy Shevchenko]

Instead of calling the ACPI specific APIs, use device_get_match_data().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spidev.c | 47 ++++++++++++++++++--------------------------
 1 file changed, 19 insertions(+), 28 deletions(-)

diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index 125da8d0e719..13dea81b21be 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -8,19 +8,20 @@
  */
 
 #include <linux/init.h>
-#include <linux/module.h>
 #include <linux/ioctl.h>
 #include <linux/fs.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/list.h>
 #include <linux/errno.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 #include <linux/compat.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
-#include <linux/acpi.h>
 
 #include <linux/spi/spi.h>
 #include <linux/spi/spidev.h>
@@ -709,10 +710,12 @@ static const struct of_device_id spidev_dt_ids[] = {
 MODULE_DEVICE_TABLE(of, spidev_dt_ids);
 #endif
 
-#ifdef CONFIG_ACPI
-
 /* Dummy SPI devices not to be used in production systems */
-#define SPIDEV_ACPI_DUMMY	1
+static int spidev_acpi_check(struct device *dev)
+{
+	dev_warn(dev, "do not use this driver in production systems!\n");
+	return 0;
+}
 
 static const struct acpi_device_id spidev_acpi_ids[] = {
 	/*
@@ -721,35 +724,18 @@ static const struct acpi_device_id spidev_acpi_ids[] = {
 	 * description of the connected peripheral and they should also use
 	 * a proper driver instead of poking directly to the SPI bus.
 	 */
-	{ "SPT0001", SPIDEV_ACPI_DUMMY },
-	{ "SPT0002", SPIDEV_ACPI_DUMMY },
-	{ "SPT0003", SPIDEV_ACPI_DUMMY },
+	{ "SPT0001", (kernel_ulong_t)&spidev_acpi_check },
+	{ "SPT0002", (kernel_ulong_t)&spidev_acpi_check },
+	{ "SPT0003", (kernel_ulong_t)&spidev_acpi_check },
 	{},
 };
 MODULE_DEVICE_TABLE(acpi, spidev_acpi_ids);
 
-static void spidev_probe_acpi(struct spi_device *spi)
-{
-	const struct acpi_device_id *id;
-
-	if (!has_acpi_companion(&spi->dev))
-		return;
-
-	id = acpi_match_device(spidev_acpi_ids, &spi->dev);
-	if (WARN_ON(!id))
-		return;
-
-	if (id->driver_data == SPIDEV_ACPI_DUMMY)
-		dev_warn(&spi->dev, "do not use this driver in production systems!\n");
-}
-#else
-static inline void spidev_probe_acpi(struct spi_device *spi) {}
-#endif
-
 /*-------------------------------------------------------------------------*/
 
 static int spidev_probe(struct spi_device *spi)
 {
+	int (*match)(struct device *dev);
 	struct spidev_data	*spidev;
 	int			status;
 	unsigned long		minor;
@@ -764,7 +750,12 @@ static int spidev_probe(struct spi_device *spi)
 		return -EINVAL;
 	}
 
-	spidev_probe_acpi(spi);
+	match = device_get_match_data(&spi->dev);
+	if (match) {
+		status = match(&spi->dev);
+		if (status)
+			return status;
+	}
 
 	/* Allocate driver data */
 	spidev = kzalloc(sizeof(*spidev), GFP_KERNEL);
@@ -834,7 +825,7 @@ static struct spi_driver spidev_spi_driver = {
 	.driver = {
 		.name =		"spidev",
 		.of_match_table = of_match_ptr(spidev_dt_ids),
-		.acpi_match_table = ACPI_PTR(spidev_acpi_ids),
+		.acpi_match_table = spidev_acpi_ids,
 	},
 	.probe =	spidev_probe,
 	.remove =	spidev_remove,
-- 
2.35.1

[PATCH v1 4/4] spidev: Replace OF specific code by device property API

[Andy Shevchenko]

Instead of calling the OF specific APIs, use device property ones.

It also prevents misusing PRP0001 in ACPI when trying to instantiate
spidev directly. We only support special SPI test devices there.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spidev.c | 45 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index 13dea81b21be..ffa124665cf3 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -20,8 +20,6 @@
 #include <linux/property.h>
 #include <linux/slab.h>
 #include <linux/compat.h>
-#include <linux/of.h>
-#include <linux/of_device.h>
 
 #include <linux/spi/spi.h>
 #include <linux/spi/spidev.h>
@@ -695,20 +693,31 @@ static const struct spi_device_id spidev_spi_ids[] = {
 };
 MODULE_DEVICE_TABLE(spi, spidev_spi_ids);
 
-#ifdef CONFIG_OF
+/*
+ * spidev should never be referenced in DT without a specific compatible string,
+ * it is a Linux implementation thing rather than a description of the hardware.
+ */
+static int spidev_of_check(struct device *dev)
+{
+	if (device_property_match_string(dev, "compatible", "spidev") < 0)
+		return 0;
+
+	dev_err(dev, "spidev listed directly in DT is not supported\n");
+	return -EINVAL;
+}
+
 static const struct of_device_id spidev_dt_ids[] = {
-	{ .compatible = "rohm,dh2228fv" },
-	{ .compatible = "lineartechnology,ltc2488" },
-	{ .compatible = "semtech,sx1301" },
-	{ .compatible = "lwn,bk4" },
-	{ .compatible = "dh,dhcom-board" },
-	{ .compatible = "menlo,m53cpld" },
-	{ .compatible = "cisco,spi-petra" },
-	{ .compatible = "micron,spi-authenta" },
+	{ .compatible = "rohm,dh2228fv", .data = &spidev_of_check },
+	{ .compatible = "lineartechnology,ltc2488", .data = &spidev_of_check },
+	{ .compatible = "semtech,sx1301", .data = &spidev_of_check },
+	{ .compatible = "lwn,bk4", .data = &spidev_of_check },
+	{ .compatible = "dh,dhcom-board", .data = &spidev_of_check },
+	{ .compatible = "menlo,m53cpld", .data = &spidev_of_check },
+	{ .compatible = "cisco,spi-petra", .data = &spidev_of_check },
+	{ .compatible = "micron,spi-authenta", .data = &spidev_of_check },
 	{},
 };
 MODULE_DEVICE_TABLE(of, spidev_dt_ids);
-#endif
 
 /* Dummy SPI devices not to be used in production systems */
 static int spidev_acpi_check(struct device *dev)
@@ -740,16 +749,6 @@ static int spidev_probe(struct spi_device *spi)
 	int			status;
 	unsigned long		minor;
 
-	/*
-	 * spidev should never be referenced in DT without a specific
-	 * compatible string, it is a Linux implementation thing
-	 * rather than a description of the hardware.
-	 */
-	if (spi->dev.of_node && of_device_is_compatible(spi->dev.of_node, "spidev")) {
-		dev_err(&spi->dev, "spidev listed directly in DT is not supported\n");
-		return -EINVAL;
-	}
-
 	match = device_get_match_data(&spi->dev);
 	if (match) {
 		status = match(&spi->dev);
@@ -824,7 +823,7 @@ static void spidev_remove(struct spi_device *spi)
 static struct spi_driver spidev_spi_driver = {
 	.driver = {
 		.name =		"spidev",
-		.of_match_table = of_match_ptr(spidev_dt_ids),
+		.of_match_table = spidev_dt_ids,
 		.acpi_match_table = spidev_acpi_ids,
 	},
 	.probe =	spidev_probe,
-- 
2.35.1

Re: [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 459 bytes --]

On Wed, Mar 23, 2022 at 04:02:12PM +0200, Andy Shevchenko wrote:
> There is no need to use atomic bit operations when allocating a minor
> number since it's done under a mutex. Moreover, one of the operations
> that is in use is non-atomic anyway.

>  	if (status == 0) {
> -		set_bit(minor, minors);
> +		__set_bit(minor, minors);
>  		list_add(&spidev->device_entry, &device_list);

There's no *need* but the __ looks suspicious...  what's the upside
here?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor

[Andy Shevchenko]

On Wed, Mar 23, 2022 at 04:39:01PM +0000, Mark Brown wrote:
> On Wed, Mar 23, 2022 at 04:02:12PM +0200, Andy Shevchenko wrote:
> > There is no need to use atomic bit operations when allocating a minor
> > number since it's done under a mutex. Moreover, one of the operations
> > that is in use is non-atomic anyway.

...

> >  	if (status == 0) {
> > -		set_bit(minor, minors);
> > +		__set_bit(minor, minors);
> >  		list_add(&spidev->device_entry, &device_list);
> 
> There's no *need* but the __ looks suspicious...  what's the upside
> here?

It's exactly what is written in the commit message

__*_bit() are non-atomic
*_bit() are atomic

Since they are wrapped by mutex, the atomic ones are not needed.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 440 bytes --]

On Wed, Mar 23, 2022 at 07:22:52PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 23, 2022 at 04:39:01PM +0000, Mark Brown wrote:

> > There's no *need* but the __ looks suspicious...  what's the upside
> > here?

> It's exactly what is written in the commit message

> __*_bit() are non-atomic
> *_bit() are atomic

> Since they are wrapped by mutex, the atomic ones are not needed.

Yes, it's not needed but what meaningful harm does it do?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor

[Andy Shevchenko]

On Wed, Mar 23, 2022 at 07:06:25PM +0000, Mark Brown wrote:
> On Wed, Mar 23, 2022 at 07:22:52PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 23, 2022 at 04:39:01PM +0000, Mark Brown wrote:
> 
> > > There's no *need* but the __ looks suspicious...  what's the upside
> > > here?
> 
> > It's exactly what is written in the commit message
> 
> > __*_bit() are non-atomic
> > *_bit() are atomic
> 
> > Since they are wrapped by mutex, the atomic ones are not needed.
> 
> Yes, it's not needed but what meaningful harm does it do?

There are basically two points:

1) in one driver the additional lock may not be influential, but
   if many drivers will do the same, it will block CPUs for no
   purpose;

2) derived from the above, if one copies'n'pastes the code, esp.
   using spin locks, it may become an unneeded code and performance
   degradation.



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 766 bytes --]

On Thu, Mar 24, 2022 at 11:24:26AM +0200, Andy Shevchenko wrote:
> On Wed, Mar 23, 2022 at 07:06:25PM +0000, Mark Brown wrote:

> > Yes, it's not needed but what meaningful harm does it do?

> There are basically two points:

> 1) in one driver the additional lock may not be influential, but
>    if many drivers will do the same, it will block CPUs for no
>    purpose;

> 2) derived from the above, if one copies'n'pastes the code, esp.
>    using spin locks, it may become an unneeded code and performance
>    degradation.

I think if these are serious issues they need to be addressed in the API
so that code doing the fancy unlocked stuff that needs atomicity is the
code that has the __ and looks like it's doing something tricky and
peering into internals.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor

[Andy Shevchenko]

On Thu, Mar 24, 2022 at 10:48 PM Mark Brown <broonie@kernel.org> wrote:
> On Thu, Mar 24, 2022 at 11:24:26AM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 23, 2022 at 07:06:25PM +0000, Mark Brown wrote:
>
> > > Yes, it's not needed but what meaningful harm does it do?
>
> > There are basically two points:
>
> > 1) in one driver the additional lock may not be influential, but
> >    if many drivers will do the same, it will block CPUs for no
> >    purpose;
>
> > 2) derived from the above, if one copies'n'pastes the code, esp.
> >    using spin locks, it may become an unneeded code and performance
> >    degradation.
>
> I think if these are serious issues they need to be addressed in the API
> so that code doing the fancy unlocked stuff that needs atomicity is the
> code that has the __ and looks like it's doing something tricky and
> peering into internals.

I believe the issue you mainly pointed out is the __ in the name of
the APIs, since it's case by case when you need one or the other. In
case of spidev we need non-atomic versions, in case of, e.g.,
drivers/dma/dw/core.c we need atomic, because spin locks used there do
not (and IIRC may not) cover some cases where the bit operations are
used against same bitmap.

Perhaps we might add the aliases as clear_bit_nonatomic() et al. Yury,
what do you think?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor

[Yury Norov]

On Fri, Mar 25, 2022 at 01:09:36PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 24, 2022 at 10:48 PM Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Mar 24, 2022 at 11:24:26AM +0200, Andy Shevchenko wrote:
> > > On Wed, Mar 23, 2022 at 07:06:25PM +0000, Mark Brown wrote:
> >
> > > > Yes, it's not needed but what meaningful harm does it do?
> >
> > > There are basically two points:
> >
> > > 1) in one driver the additional lock may not be influential, but
> > >    if many drivers will do the same, it will block CPUs for no
> > >    purpose;
> >
> > > 2) derived from the above, if one copies'n'pastes the code, esp.
> > >    using spin locks, it may become an unneeded code and performance
> > >    degradation.
> >
> > I think if these are serious issues they need to be addressed in the API
> > so that code doing the fancy unlocked stuff that needs atomicity is the
> > code that has the __ and looks like it's doing something tricky and
> > peering into internals.
> 
> I believe the issue you mainly pointed out is the __ in the name of
> the APIs, since it's case by case when you need one or the other. In
> case of spidev we need non-atomic versions, in case of, e.g.,
> drivers/dma/dw/core.c we need atomic, because spin locks used there do
> not (and IIRC may not) cover some cases where the bit operations are
> used against same bitmap.
> 
> Perhaps we might add the aliases as clear_bit_nonatomic() et al. Yury,
> what do you think?

We already have bitmap_clear(addr, nr, 1), which would call
__clear_bit() without any overhead. So, I think we'd encourage people
to switch to bitmap_{set,clear} where it makes sense, i.e. where the
object is a real bitmap, not a thing like flags. We can even invent
bitmap_set_bit(addr, nr) if needed.

What really concerns me is that our atomic bit operations are not
really atomic. If bitmap doesn't fit into a single word, different
threads may read/write different parts of such bitmap concurrently. 

Another thing is that we have no atomic versions for functions like
bitmap_empty(), which means that atomic part of bitmap API cannot be
mixed with non-atomic part.

This means that atomic ops are most probably used wider than it worth.
I don't know how many useless atomic bitmap ops we have, I also don't
know how to estimate the excessive pressure on cache subsystem generated
by this useless clear/set_bit() traffic. 

Thanks,
Yury

Re: [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor

[Andy Shevchenko]

On Wed, Mar 23, 2022 at 04:02:12PM +0200, Andy Shevchenko wrote:
> There is no need to use atomic bit operations when allocating a minor
> number since it's done under a mutex. Moreover, one of the operations
> that is in use is non-atomic anyway.

Shall I resend the series without this patch, or can you apply the rest
if you have no comments / objections?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 467 bytes --]

On Fri, Mar 25, 2022 at 02:44:34PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 23, 2022 at 04:02:12PM +0200, Andy Shevchenko wrote:
> > There is no need to use atomic bit operations when allocating a minor
> > number since it's done under a mutex. Moreover, one of the operations
> > that is in use is non-atomic anyway.

> Shall I resend the series without this patch, or can you apply the rest
> if you have no comments / objections?

I've already queued the rest.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor

[Andy Shevchenko]

On Fri, Mar 25, 2022 at 01:26:51PM +0000, Mark Brown wrote:
> On Fri, Mar 25, 2022 at 02:44:34PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 23, 2022 at 04:02:12PM +0200, Andy Shevchenko wrote:

...

> > Shall I resend the series without this patch, or can you apply the rest
> > if you have no comments / objections?
> 
> I've already queued the rest.

Thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: (subset) [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor

[Mark Brown]

On Wed, 23 Mar 2022 16:02:12 +0200, Andy Shevchenko wrote:
> There is no need to use atomic bit operations when allocating a minor
> number since it's done under a mutex. Moreover, one of the operations
> that is in use is non-atomic anyway.
> 
> 

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[2/4] spidev: Convert BUILD_BUG_ON() to static_assert()
      commit: d21b94bf3ac44aa7759c0de6f72c0a887eb9e23b
[3/4] spidev: Replace ACPI specific code by device_get_match_data()
      commit: 2a7f669dd8f6561d227e724ca2614c25732f4799
[4/4] spidev: Replace OF specific code by device property API
      commit: 88a285192084edab6657e819f7f130f9cfcb0579

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark