linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor
@ 2022-03-23 14:02 Andy Shevchenko
  2022-03-23 14:02 ` [PATCH v1 2/4] spidev: Convert BUILD_BUG_ON() to static_assert() Andy Shevchenko
                   ` (5 more replies)
  0 siblings, 6 replies; 15+ messages in thread
From: Andy Shevchenko @ 2022-03-23 14:02 UTC (permalink / raw)
  To: Andy Shevchenko, linux-spi, linux-kernel; +Cc: Mark Brown

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v1 2/4] spidev: Convert BUILD_BUG_ON() to static_assert()
  2022-03-23 14:02 [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor Andy Shevchenko
@ 2022-03-23 14:02 ` Andy Shevchenko
  2022-03-23 14:02 ` [PATCH v1 3/4] spidev: Replace ACPI specific code by device_get_match_data() Andy Shevchenko
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2022-03-23 14:02 UTC (permalink / raw)
  To: Andy Shevchenko, linux-spi, linux-kernel; +Cc: Mark Brown

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v1 3/4] spidev: Replace ACPI specific code by device_get_match_data()
  2022-03-23 14:02 [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor Andy Shevchenko
  2022-03-23 14:02 ` [PATCH v1 2/4] spidev: Convert BUILD_BUG_ON() to static_assert() Andy Shevchenko
@ 2022-03-23 14:02 ` Andy Shevchenko
  2022-03-23 14:02 ` [PATCH v1 4/4] spidev: Replace OF specific code by device property API Andy Shevchenko
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2022-03-23 14:02 UTC (permalink / raw)
  To: Andy Shevchenko, linux-spi, linux-kernel; +Cc: Mark Brown

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v1 4/4] spidev: Replace OF specific code by device property API
  2022-03-23 14:02 [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor Andy Shevchenko
  2022-03-23 14:02 ` [PATCH v1 2/4] spidev: Convert BUILD_BUG_ON() to static_assert() Andy Shevchenko
  2022-03-23 14:02 ` [PATCH v1 3/4] spidev: Replace ACPI specific code by device_get_match_data() Andy Shevchenko
@ 2022-03-23 14:02 ` Andy Shevchenko
  2022-03-23 16:39 ` [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor Mark Brown
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2022-03-23 14:02 UTC (permalink / raw)
  To: Andy Shevchenko, linux-spi, linux-kernel; +Cc: Mark Brown

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor
  2022-03-23 14:02 [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor Andy Shevchenko
                   ` (2 preceding siblings ...)
  2022-03-23 14:02 ` [PATCH v1 4/4] spidev: Replace OF specific code by device property API Andy Shevchenko
@ 2022-03-23 16:39 ` Mark Brown
  2022-03-23 17:22   ` Andy Shevchenko
  2022-03-25 12:44 ` Andy Shevchenko
  2022-04-05  9:32 ` (subset) " Mark Brown
  5 siblings, 1 reply; 15+ messages in thread
From: Mark Brown @ 2022-03-23 16:39 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-spi, linux-kernel

[-- 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 --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor
  2022-03-23 16:39 ` [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor Mark Brown
@ 2022-03-23 17:22   ` Andy Shevchenko
  2022-03-23 19:06     ` Mark Brown
  0 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2022-03-23 17:22 UTC (permalink / raw)
  To: Mark Brown; +Cc: linux-spi, linux-kernel

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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor
  2022-03-23 17:22   ` Andy Shevchenko
@ 2022-03-23 19:06     ` Mark Brown
  2022-03-24  9:24       ` Andy Shevchenko
  0 siblings, 1 reply; 15+ messages in thread
From: Mark Brown @ 2022-03-23 19:06 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-spi, linux-kernel

[-- 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 --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor
  2022-03-23 19:06     ` Mark Brown
@ 2022-03-24  9:24       ` Andy Shevchenko
  2022-03-24 14:28         ` Mark Brown
  0 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2022-03-24  9:24 UTC (permalink / raw)
  To: Mark Brown; +Cc: linux-spi, linux-kernel

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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor
  2022-03-24  9:24       ` Andy Shevchenko
@ 2022-03-24 14:28         ` Mark Brown
  2022-03-25 11:09           ` Andy Shevchenko
  0 siblings, 1 reply; 15+ messages in thread
From: Mark Brown @ 2022-03-24 14:28 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-spi, linux-kernel

[-- 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 --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor
  2022-03-24 14:28         ` Mark Brown
@ 2022-03-25 11:09           ` Andy Shevchenko
  2022-03-26  1:29             ` Yury Norov
  0 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2022-03-25 11:09 UTC (permalink / raw)
  To: Mark Brown, Yury Norov
  Cc: Andy Shevchenko, linux-spi, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor
  2022-03-23 14:02 [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor Andy Shevchenko
                   ` (3 preceding siblings ...)
  2022-03-23 16:39 ` [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor Mark Brown
@ 2022-03-25 12:44 ` Andy Shevchenko
  2022-03-25 13:26   ` Mark Brown
  2022-04-05  9:32 ` (subset) " Mark Brown
  5 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2022-03-25 12:44 UTC (permalink / raw)
  To: linux-spi, linux-kernel; +Cc: Mark Brown

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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor
  2022-03-25 12:44 ` Andy Shevchenko
@ 2022-03-25 13:26   ` Mark Brown
  2022-03-25 13:44     ` Andy Shevchenko
  0 siblings, 1 reply; 15+ messages in thread
From: Mark Brown @ 2022-03-25 13:26 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-spi, linux-kernel

[-- 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 --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor
  2022-03-25 13:26   ` Mark Brown
@ 2022-03-25 13:44     ` Andy Shevchenko
  0 siblings, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2022-03-25 13:44 UTC (permalink / raw)
  To: Mark Brown; +Cc: linux-spi, linux-kernel

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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor
  2022-03-25 11:09           ` Andy Shevchenko
@ 2022-03-26  1:29             ` Yury Norov
  0 siblings, 0 replies; 15+ messages in thread
From: Yury Norov @ 2022-03-26  1:29 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Mark Brown, Andy Shevchenko, linux-spi, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: (subset) [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor
  2022-03-23 14:02 [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor Andy Shevchenko
                   ` (4 preceding siblings ...)
  2022-03-25 12:44 ` Andy Shevchenko
@ 2022-04-05  9:32 ` Mark Brown
  5 siblings, 0 replies; 15+ messages in thread
From: Mark Brown @ 2022-04-05  9:32 UTC (permalink / raw)
  To: linux-kernel, linux-spi, andriy.shevchenko

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2022-04-05 15:21 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-23 14:02 [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor Andy Shevchenko
2022-03-23 14:02 ` [PATCH v1 2/4] spidev: Convert BUILD_BUG_ON() to static_assert() Andy Shevchenko
2022-03-23 14:02 ` [PATCH v1 3/4] spidev: Replace ACPI specific code by device_get_match_data() Andy Shevchenko
2022-03-23 14:02 ` [PATCH v1 4/4] spidev: Replace OF specific code by device property API Andy Shevchenko
2022-03-23 16:39 ` [PATCH v1 1/4] spidev: Do not use atomic bit operations when allocating minor Mark Brown
2022-03-23 17:22   ` Andy Shevchenko
2022-03-23 19:06     ` Mark Brown
2022-03-24  9:24       ` Andy Shevchenko
2022-03-24 14:28         ` Mark Brown
2022-03-25 11:09           ` Andy Shevchenko
2022-03-26  1:29             ` Yury Norov
2022-03-25 12:44 ` Andy Shevchenko
2022-03-25 13:26   ` Mark Brown
2022-03-25 13:44     ` Andy Shevchenko
2022-04-05  9:32 ` (subset) " Mark Brown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).