w1: simplify allocation of w1_master

[PATCH] w1: simplify allocation of w1_master

Posted by Rosen Penev 1 week, 3 days ago

Convert bus_master to a flexible array member as it's being used as one
with the old C89 + 1 pointer trick.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/w1/w1_int.c | 4 +---
 include/linux/w1.h  | 4 ++--
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/w1/w1_int.c b/drivers/w1/w1_int.c
index 19a0ea28e9f3..a0bb97608564 100644
--- a/drivers/w1/w1_int.c
+++ b/drivers/w1/w1_int.c
@@ -31,12 +31,10 @@ static struct w1_master *w1_alloc_dev(u32 id, int slave_count, int slave_ttl,
 	/*
 	 * We are in process context(kernel thread), so can sleep.
 	 */
-	dev = kzalloc(sizeof(struct w1_master) + sizeof(struct w1_bus_master), GFP_KERNEL);
+	dev = kzalloc_flex(*dev, bus_master, 1);
 	if (!dev)
 		return NULL;

-	dev->bus_master = (struct w1_bus_master *)(dev + 1);
-
 	dev->owner		= THIS_MODULE;
 	dev->max_slave_count	= slave_count;
 	dev->slave_count	= 0;
diff --git a/include/linux/w1.h b/include/linux/w1.h
index 064805bfae3f..7379a9ddefc0 100644
--- a/include/linux/w1.h
+++ b/include/linux/w1.h
@@ -236,9 +236,9 @@ struct w1_master {
 	struct device_driver	*driver;
 	struct device		dev;

-	struct w1_bus_master	*bus_master;
-
 	u32			seq;
+
+	struct w1_bus_master	bus_master[];
 };

 int w1_add_master_device(struct w1_bus_master *master);
--
2.53.0

Re: [PATCH] w1: simplify allocation of w1_master

Posted by Krzysztof Kozlowski 1 week ago

On 24/03/2026 03:15, Rosen Penev wrote:
> Convert bus_master to a flexible array member as it's being used as one
> with the old C89 + 1 pointer trick.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/w1/w1_int.c | 4 +---
>  include/linux/w1.h  | 4 ++--
>  2 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/w1/w1_int.c b/drivers/w1/w1_int.c
> index 19a0ea28e9f3..a0bb97608564 100644
> --- a/drivers/w1/w1_int.c
> +++ b/drivers/w1/w1_int.c
> @@ -31,12 +31,10 @@ static struct w1_master *w1_alloc_dev(u32 id, int slave_count, int slave_ttl,
>  	/*
>  	 * We are in process context(kernel thread), so can sleep.
>  	 */
> -	dev = kzalloc(sizeof(struct w1_master) + sizeof(struct w1_bus_master), GFP_KERNEL);
> +	dev = kzalloc_flex(*dev, bus_master, 1);
>  	if (!dev)
>  		return NULL;
> 
> -	dev->bus_master = (struct w1_bus_master *)(dev + 1);
> -
>  	dev->owner		= THIS_MODULE;
>  	dev->max_slave_count	= slave_count;
>  	dev->slave_count	= 0;
> diff --git a/include/linux/w1.h b/include/linux/w1.h
> index 064805bfae3f..7379a9ddefc0 100644
> --- a/include/linux/w1.h
> +++ b/include/linux/w1.h
> @@ -236,9 +236,9 @@ struct w1_master {
>  	struct device_driver	*driver;
>  	struct device		dev;
> 
> -	struct w1_bus_master	*bus_master;
> -
>  	u32			seq;
> +
> +	struct w1_bus_master	bus_master[];

Code does not use it as flexible array member, does it? Why this cannot
be just two kzalloc's - one for w1_master and then next for bus_master?
Or embed it directly (composition) in w1_master?

Best regards,
Krzysztof

Re: [PATCH] w1: simplify allocation of w1_master

Posted by Rosen Penev 1 week ago

On Thu, Mar 26, 2026 at 3:31 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 24/03/2026 03:15, Rosen Penev wrote:
> > Convert bus_master to a flexible array member as it's being used as one
> > with the old C89 + 1 pointer trick.
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >  drivers/w1/w1_int.c | 4 +---
> >  include/linux/w1.h  | 4 ++--
> >  2 files changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/w1/w1_int.c b/drivers/w1/w1_int.c
> > index 19a0ea28e9f3..a0bb97608564 100644
> > --- a/drivers/w1/w1_int.c
> > +++ b/drivers/w1/w1_int.c
> > @@ -31,12 +31,10 @@ static struct w1_master *w1_alloc_dev(u32 id, int slave_count, int slave_ttl,
> >       /*
> >        * We are in process context(kernel thread), so can sleep.
> >        */
> > -     dev = kzalloc(sizeof(struct w1_master) + sizeof(struct w1_bus_master), GFP_KERNEL);
> > +     dev = kzalloc_flex(*dev, bus_master, 1);
> >       if (!dev)
> >               return NULL;
> >
> > -     dev->bus_master = (struct w1_bus_master *)(dev + 1);
> > -
> >       dev->owner              = THIS_MODULE;
> >       dev->max_slave_count    = slave_count;
> >       dev->slave_count        = 0;
> > diff --git a/include/linux/w1.h b/include/linux/w1.h
> > index 064805bfae3f..7379a9ddefc0 100644
> > --- a/include/linux/w1.h
> > +++ b/include/linux/w1.h
> > @@ -236,9 +236,9 @@ struct w1_master {
> >       struct device_driver    *driver;
> >       struct device           dev;
> >
> > -     struct w1_bus_master    *bus_master;
> > -
> >       u32                     seq;
> > +
> > +     struct w1_bus_master    bus_master[];
>
> Code does not use it as flexible array member, does it? Why this cannot
> be just two kzalloc's - one for w1_master and then next for bus_master?
> Or embed it directly (composition) in w1_master?
two kzalloc or kzalloc + kcalloc just does two allocations needlessly.

It can be embedded, yes.
>
> Best regards,
> Krzysztof

Re: [PATCH] w1: simplify allocation of w1_master

Posted by Krzysztof Kozlowski 1 week ago

On 26/03/2026 20:08, Rosen Penev wrote:
> On Thu, Mar 26, 2026 at 3:31 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 24/03/2026 03:15, Rosen Penev wrote:
>>> Convert bus_master to a flexible array member as it's being used as one
>>> with the old C89 + 1 pointer trick.
>>>
>>> Signed-off-by: Rosen Penev <rosenp@gmail.com>
>>> ---
>>>  drivers/w1/w1_int.c | 4 +---
>>>  include/linux/w1.h  | 4 ++--
>>>  2 files changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/w1/w1_int.c b/drivers/w1/w1_int.c
>>> index 19a0ea28e9f3..a0bb97608564 100644
>>> --- a/drivers/w1/w1_int.c
>>> +++ b/drivers/w1/w1_int.c
>>> @@ -31,12 +31,10 @@ static struct w1_master *w1_alloc_dev(u32 id, int slave_count, int slave_ttl,
>>>       /*
>>>        * We are in process context(kernel thread), so can sleep.
>>>        */
>>> -     dev = kzalloc(sizeof(struct w1_master) + sizeof(struct w1_bus_master), GFP_KERNEL);
>>> +     dev = kzalloc_flex(*dev, bus_master, 1);
>>>       if (!dev)
>>>               return NULL;
>>>
>>> -     dev->bus_master = (struct w1_bus_master *)(dev + 1);
>>> -
>>>       dev->owner              = THIS_MODULE;
>>>       dev->max_slave_count    = slave_count;
>>>       dev->slave_count        = 0;
>>> diff --git a/include/linux/w1.h b/include/linux/w1.h
>>> index 064805bfae3f..7379a9ddefc0 100644
>>> --- a/include/linux/w1.h
>>> +++ b/include/linux/w1.h
>>> @@ -236,9 +236,9 @@ struct w1_master {
>>>       struct device_driver    *driver;
>>>       struct device           dev;
>>>
>>> -     struct w1_bus_master    *bus_master;
>>> -
>>>       u32                     seq;
>>> +
>>> +     struct w1_bus_master    bus_master[];
>>
>> Code does not use it as flexible array member, does it? Why this cannot
>> be just two kzalloc's - one for w1_master and then next for bus_master?
>> Or embed it directly (composition) in w1_master?
> two kzalloc or kzalloc + kcalloc just does two allocations needlessly.

Needless is marking something as flexible array while it is not such.
You write readable and obvious code, not something to optimize number of
allocs.

> 
> It can be embedded, yes.
>>
>> Best regards,
>> Krzysztof


Best regards,
Krzysztof