Re: [PATCH] driver core: Fix size calculation of symlink name for devlink_(add|remove)_symlinks()

[quic_zijuhu <quic_zijuhu@quicinc.com>]

On 7/9/2024 6:43 AM, Saravana Kannan wrote:
> On Sun, Jul 7, 2024 at 6:24 AM Zijun Hu <zijun_hu@icloud.com> wrote:
>>
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> devlink_(add|remove)_symlinks() wants to kzalloc() memory to save symlink
>> name for either supplier or consumer, but forget to consider consumer
>> prefix when calclulate memory size, fixed by considering prefix for both
>> supplier and consumer.
> 
> No, I didn't forget to take "consumer" into account :) Both supplier
> and consumer are the same length. So I didn't bother doing both. I
> don't see a point behind this patch.
> 
it is not obvious for code readers that "supplier:" and "consumer:" have
the same string length.

code readers maybe need to count characters one by one for both strings
to confirm both have the same length.
>>
>> Fixes: 287905e68dd2 ("driver core: Expose device link details in sysfs")
> 
> It's definitely not "Fixing" anything because nothing is broken.
>this change maybe fix algorithm or procedures to calculate the size to
kzalloc() even if it don't change the resulted size.

> Nack.
>> If you really want this in, remove this tag and send it again. I won't
> ack or review it though as I don't think it adds much value. Greg can
> take it if he thinks he likes it.
> 
okay, will send v2 which will remove the fix tag, i feels this change is
worthy due to below reasons:

1) readers is easier to understand the algorithm or procedures to
calculate the resulted size.

2) readers don't need to take extra effort to confirm that fact that
both string have the same length.

let us wait for Greg's opinions.
> -Saravana
> 
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>>  drivers/base/core.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index 2b4c0624b704..f14cfe5c97b7 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -572,7 +572,7 @@ static int devlink_add_symlinks(struct device *dev)
>>         len = max(strlen(dev_bus_name(sup)) + strlen(dev_name(sup)),
>>                   strlen(dev_bus_name(con)) + strlen(dev_name(con)));
>>         len += strlen(":");
>> -       len += strlen("supplier:") + 1;
>> +       len += max(strlen("supplier:"), strlen("consumer:")) + 1;
>>         buf = kzalloc(len, GFP_KERNEL);
>>         if (!buf)
>>                 return -ENOMEM;
>> @@ -623,7 +623,7 @@ static void devlink_remove_symlinks(struct device *dev)
>>         len = max(strlen(dev_bus_name(sup)) + strlen(dev_name(sup)),
>>                   strlen(dev_bus_name(con)) + strlen(dev_name(con)));
>>         len += strlen(":");
>> -       len += strlen("supplier:") + 1;
>> +       len += max(strlen("supplier:"), strlen("consumer:")) + 1;
>>         buf = kzalloc(len, GFP_KERNEL);
>>         if (!buf) {
>>                 WARN(1, "Unable to properly free device link symlinks!\n");
>>
>> ---
>> base-commit: c6653f49e4fd3b0d52c12a1fc814d6c5b234ea15
>> change-id: 20240707-devlink_fix-0fa46dedfe95
>>
>> Best regards,
>> --
>> Zijun Hu <quic_zijuhu@quicinc.com>
>>