[PATCH v2 0/3] Allow specifying properties with spi_register_board_info

[PATCH v2 0/3] Allow specifying properties with spi_register_board_info

[Dmitry Torokhov]

I am trying to convert some input drivers to use only generic device
properties, and get rid of legacy platform data. For that I need to be
able to attach property lists to devices before they are registered, and
quite a few boards use spi_register_board_info() to create their devices.

v2: change patch #2 from kmemdup to kmalloc + copy as they are different
    data types.

v1: initial posting

Dmitry Torokhov (3):
  spi: allow attaching device properties to SPI board info
  spi: allocate spi_board_info entries one by one
  spi: allow registering empty spi_board_info lists

 drivers/spi/spi.c       | 40 ++++++++++++++++++++++++++++------------
 include/linux/spi/spi.h |  4 ++++
 2 files changed, 32 insertions(+), 12 deletions(-)

-- 
Dmitry

[PATCH v2 1/3] spi: allow attaching device properties to SPI board info

[Dmitry Torokhov]

Generic device properties support statically defined property sets. For
them to be usable, we need to attach these property sets before devices
are registered and probed. Allowing to attach property list to
spi_board_info structure will allow non-ACPI non-DT boards switch to using
generic properties and get rid of custom platform data.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/spi/spi.c       | 27 +++++++++++++++++++++++----
 include/linux/spi/spi.h |  4 ++++
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 656dd3e3220c..b948d8cdbace 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -31,6 +31,7 @@
 #include <linux/of_gpio.h>
 #include <linux/pm_runtime.h>
 #include <linux/pm_domain.h>
+#include <linux/property.h>
 #include <linux/export.h>
 #include <linux/sched/rt.h>
 #include <linux/delay.h>
@@ -599,13 +600,25 @@ struct spi_device *spi_new_device(struct spi_master *master,
 	proxy->controller_data = chip->controller_data;
 	proxy->controller_state = NULL;
 
-	status = spi_add_device(proxy);
-	if (status < 0) {
-		spi_dev_put(proxy);
-		return NULL;
+	if (chip->properties) {
+		status = device_add_properties(&proxy->dev, chip->properties);
+		if (status) {
+			dev_err(&master->dev,
+				"failed to add properties to '%s': %d\n",
+				chip->modalias, status);
+			goto err_out;
+		}
 	}
 
+	status = spi_add_device(proxy);
+	if (status < 0)
+		goto err_out;
+
 	return proxy;
+
+err_out:
+	spi_dev_put(proxy);
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(spi_new_device);
 
@@ -661,6 +674,7 @@ static void spi_match_master_to_boardinfo(struct spi_master *master,
  *
  * The board info passed can safely be __initdata ... but be careful of
  * any embedded pointers (platform_data, etc), they're copied as-is.
+ * Device properties are deep-copied though.
  *
  * Return: zero on success, else a negative error code.
  */
@@ -680,6 +694,11 @@ int spi_register_board_info(struct spi_board_info const *info, unsigned n)
 		struct spi_master *master;
 
 		memcpy(&bi->board_info, info, sizeof(*info));
+		bi->board_info.properties =
+			property_entries_dup(info->properties);
+		if (IS_ERR(bi->board_info.properties))
+			return PTR_ERR(bi->board_info.properties);
+
 		mutex_lock(&board_lock);
 		list_add_tail(&bi->list, &board_list);
 		list_for_each_entry(master, &spi_master_list, list)
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 75c6bd0ac605..5a8c4b24f2dc 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -23,6 +23,7 @@
 #include <linux/scatterlist.h>
 
 struct dma_chan;
+struct property_entry;
 struct spi_master;
 struct spi_transfer;
 struct spi_flash_read_message;
@@ -1209,6 +1210,7 @@ int spi_flash_read(struct spi_device *spi,
  * @modalias: Initializes spi_device.modalias; identifies the driver.
  * @platform_data: Initializes spi_device.platform_data; the particular
  *	data stored there is driver-specific.
+ * @properties: Additional device properties for the device.
  * @controller_data: Initializes spi_device.controller_data; some
  *	controllers need hints about hardware setup, e.g. for DMA.
  * @irq: Initializes spi_device.irq; depends on how the board is wired.
@@ -1241,10 +1243,12 @@ struct spi_board_info {
 	 *
 	 * platform_data goes to spi_device.dev.platform_data,
 	 * controller_data goes to spi_device.controller_data,
+	 * device properties are copied and attached to spi_device,
 	 * irq is copied too
 	 */
 	char		modalias[SPI_NAME_SIZE];
 	const void	*platform_data;
+	const struct property_entry *properties;
 	void		*controller_data;
 	int		irq;
 
-- 
2.11.0.483.g087da7b7c-goog

[PATCH v2 2/3] spi: allocate spi_board_info entries one by one

[Dmitry Torokhov]

Lists of spi_board_info entries registered with spi_register_board_info()
can be quite long; instead of forcing memory allocator find contagious
chunk of memory, let;s allocate them one-by-one, so they can be packed as
needed.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/spi/spi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index b948d8cdbace..1b66e0497327 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -686,14 +686,14 @@ int spi_register_board_info(struct spi_board_info const *info, unsigned n)
 	if (!n)
 		return -EINVAL;
 
-	bi = kzalloc(n * sizeof(*bi), GFP_KERNEL);
-	if (!bi)
-		return -ENOMEM;
-
-	for (i = 0; i < n; i++, bi++, info++) {
+	for (i = 0; i < n; i++, info++) {
 		struct spi_master *master;
 
-		memcpy(&bi->board_info, info, sizeof(*info));
+		bi = kmalloc(sizeof(*bi), GFP_KERNEL);
+		if (!bi)
+			return -ENOMEM;
+
+		bi->board_info = *info;
 		bi->board_info.properties =
 			property_entries_dup(info->properties);
 		if (IS_ERR(bi->board_info.properties))
-- 
2.11.0.483.g087da7b7c-goog

Re: [PATCH v2 2/3] spi: allocate spi_board_info entries one by one

[Mark Brown]

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

On Mon, Feb 27, 2017 at 08:18:56PM -0800, Dmitry Torokhov wrote:
> Lists of spi_board_info entries registered with spi_register_board_info()
> can be quite long; instead of forcing memory allocator find contagious

Do you have numbers on that?

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

Re: [PATCH v2 2/3] spi: allocate spi_board_info entries one by one

[Dmitry Torokhov]

On Tue, Feb 28, 2017 at 09:16:50AM +0000, Mark Brown wrote:
> On Mon, Feb 27, 2017 at 08:18:56PM -0800, Dmitry Torokhov wrote:
> > Lists of spi_board_info entries registered with spi_register_board_info()
> > can be quite long; instead of forcing memory allocator find contagious
> 
> Do you have numbers on that?

Hm, so the largest array seems to be in
arch/blackfin/mach-bf537/boards/stamp.c at max of 43 entries. The new
board info is ether 60 or 72 bytes, so we get 2 or 3K table. Not above
page, but still could be packed I think.

If we decide that we want to keep single chunk I'll just change the
allocation to kcalloc. Let me know.

We should probably redo patch #1 to avoid allocating empty property sets
anyway.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] spi: allocate spi_board_info entries one by one

[Mark Brown]

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

On Tue, Feb 28, 2017 at 10:24:17AM -0800, Dmitry Torokhov wrote:
> On Tue, Feb 28, 2017 at 09:16:50AM +0000, Mark Brown wrote:
> > On Mon, Feb 27, 2017 at 08:18:56PM -0800, Dmitry Torokhov wrote:
> > > Lists of spi_board_info entries registered with spi_register_board_info()
> > > can be quite long; instead of forcing memory allocator find contagious

> > Do you have numbers on that?

> Hm, so the largest array seems to be in
> arch/blackfin/mach-bf537/boards/stamp.c at max of 43 entries. The new
> board info is ether 60 or 72 bytes, so we get 2 or 3K table. Not above
> page, but still could be packed I think.

Oh wow, that's impressively large.  Still not sure the optimization is
particularly worth it though, it's small change in the grand scheme of
things.  OTOH it's a small change.

> If we decide that we want to keep single chunk I'll just change the
> allocation to kcalloc. Let me know.

I'd be inclined to do that because it requires less thinking about the
value of what should be a very small optimization either way but
whatever :)

> We should probably redo patch #1 to avoid allocating empty property sets
> anyway.

Yeah.

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

Re: [PATCH v2 2/3] spi: allocate spi_board_info entries one by one

[Geert Uytterhoeven]

On Tue, Feb 28, 2017 at 7:54 PM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Feb 28, 2017 at 10:24:17AM -0800, Dmitry Torokhov wrote:
>> On Tue, Feb 28, 2017 at 09:16:50AM +0000, Mark Brown wrote:
>> > On Mon, Feb 27, 2017 at 08:18:56PM -0800, Dmitry Torokhov wrote:
>> > > Lists of spi_board_info entries registered with spi_register_board_info()
>> > > can be quite long; instead of forcing memory allocator find contagious
>
>> > Do you have numbers on that?
>
>> Hm, so the largest array seems to be in
>> arch/blackfin/mach-bf537/boards/stamp.c at max of 43 entries. The new
>> board info is ether 60 or 72 bytes, so we get 2 or 3K table. Not above
>> page, but still could be packed I think.
>
> Oh wow, that's impressively large.  Still not sure the optimization is
> particularly worth it though, it's small change in the grand scheme of
> things.  OTOH it's a small change.

Given this is done during early boot, what's the probability of not having
sufficient contiguous memory?

>> If we decide that we want to keep single chunk I'll just change the
>> allocation to kcalloc. Let me know.
>
> I'd be inclined to do that because it requires less thinking about the
> value of what should be a very small optimization either way but
> whatever :)

Tada...

commit f9bdb7fdd2cac17bdc9c344b6036e6939fa087cd
Author: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Date:   Fri Jan 13 12:28:04 2017 +0100

    spi: Use kcalloc() in spi_register_board_info()

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] spi: allocate spi_board_info entries one by one

[Dmitry Torokhov]

On Tue, Feb 28, 2017 at 09:15:12PM +0100, Geert Uytterhoeven wrote:
> On Tue, Feb 28, 2017 at 7:54 PM, Mark Brown <broonie@kernel.org> wrote:
> > On Tue, Feb 28, 2017 at 10:24:17AM -0800, Dmitry Torokhov wrote:
> >> On Tue, Feb 28, 2017 at 09:16:50AM +0000, Mark Brown wrote:
> >> > On Mon, Feb 27, 2017 at 08:18:56PM -0800, Dmitry Torokhov wrote:
> >> > > Lists of spi_board_info entries registered with spi_register_board_info()
> >> > > can be quite long; instead of forcing memory allocator find contagious
> >
> >> > Do you have numbers on that?
> >
> >> Hm, so the largest array seems to be in
> >> arch/blackfin/mach-bf537/boards/stamp.c at max of 43 entries. The new
> >> board info is ether 60 or 72 bytes, so we get 2 or 3K table. Not above
> >> page, but still could be packed I think.
> >
> > Oh wow, that's impressively large.  Still not sure the optimization is
> > particularly worth it though, it's small change in the grand scheme of
> > things.  OTOH it's a small change.
> 
> Given this is done during early boot, what's the probability of not having
> sufficient contiguous memory?
> 
> >> If we decide that we want to keep single chunk I'll just change the
> >> allocation to kcalloc. Let me know.
> >
> > I'd be inclined to do that because it requires less thinking about the
> > value of what should be a very small optimization either way but
> > whatever :)
> 
> Tada...
> 
> commit f9bdb7fdd2cac17bdc9c344b6036e6939fa087cd
> Author: Markus Elfring <elfring@users.sourceforge.net>
> Date:   Fri Jan 13 12:28:04 2017 +0100
> 
>     spi: Use kcalloc() in spi_register_board_info()

Or even kmalloc_array() as zeroing out memory is not needed. But I'll
let Mark sort it out and drop my patch #2.

Thanks.

-- 
Dmitry

Re: [PATCH v2 2/3] spi: allocate spi_board_info entries one by one

[Lars-Peter Clausen]

On 02/28/2017 07:54 PM, Mark Brown wrote:
> On Tue, Feb 28, 2017 at 10:24:17AM -0800, Dmitry Torokhov wrote:
>> On Tue, Feb 28, 2017 at 09:16:50AM +0000, Mark Brown wrote:
>>> On Mon, Feb 27, 2017 at 08:18:56PM -0800, Dmitry Torokhov wrote:
>>>> Lists of spi_board_info entries registered with spi_register_board_info()
>>>> can be quite long; instead of forcing memory allocator find contagious
> 
>>> Do you have numbers on that?
> 
>> Hm, so the largest array seems to be in
>> arch/blackfin/mach-bf537/boards/stamp.c at max of 43 entries. The new
>> board info is ether 60 or 72 bytes, so we get 2 or 3K table. Not above
>> page, but still could be packed I think.
> 
> Oh wow, that's impressively large.  Still not sure the optimization is
> particularly worth it though, it's small change in the grand scheme of
> things.  OTOH it's a small change.

The Blackfin machine files shouldn't be used as valid example. While in
theory it is possible to build a kernel with that many entries nobody is
ever going to do that. What the Blackfin machine files do is basically a
poor man's overlays from the days before overlays existed. Most of the
entries in the table use the same chip-select pins, it wouldn't be possible
to use  use a setup where more than two or three of the entries is enabled
at the same time.

[PATCH v2 3/3] spi: allow registering empty spi_board_info lists

[Dmitry Torokhov]

Many boards form list of spi_board_info entries depending on config,
and it is possible to end up with empty list. Do not report error
in such cases.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/spi/spi.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 1b66e0497327..dbc4f00dc2fc 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -683,9 +683,6 @@ int spi_register_board_info(struct spi_board_info const *info, unsigned n)
 	struct boardinfo *bi;
 	int i;
 
-	if (!n)
-		return -EINVAL;
-
 	for (i = 0; i < n; i++, info++) {
 		struct spi_master *master;
 
-- 
2.11.0.483.g087da7b7c-goog

Re: [PATCH v2 3/3] spi: allow registering empty spi_board_info lists

[Andy Shevchenko]

On Tue, Feb 28, 2017 at 6:18 AM, Dmitry Torokhov
<dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Many boards form list of spi_board_info entries depending on config,
> and it is possible to end up with empty list. Do not report error
> in such cases.

If there is an actual case already it would be better to make the
patch first in the series with Cc: stable@.

FWIW:
Reviewed-by: Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] spi: allow registering empty spi_board_info lists

[Dmitry Torokhov]

On Tue, Feb 28, 2017 at 04:01:46PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 28, 2017 at 6:18 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Many boards form list of spi_board_info entries depending on config,
> > and it is possible to end up with empty list. Do not report error
> > in such cases.
> 
> If there is an actual case already it would be better to make the
> patch first in the series with Cc: stable@.

arch/blackfin/mach-bf533/boards/ezkit.c: you can get config convoluted
enough so that you'll end up with empty array. But nobody checks the
result of the call, so the failure is not visible. There is no need to
cc stable@ for this.

> 
> FWIW:
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 

Thanks.

-- 
Dmitry