[PATCH linux 1/2] i2c: aspeed: Don't rename added devices

[Joel Stanley]

On Sun, Feb 14, 2016 at 11:05 AM, Milton Miller II <miltonm at us.ibm.com> wrote:
>
>
>
> On 02/11/2016 10:23PM, Joel Stanley wrote:
>>Subject: Re: [PATCH linux 1/2] i2c: aspeed: Don't rename added devices
>>
>>On Thu, Feb 11, 2016 at 12:00 PM, OpenBMC Patches
> <openbmc-patches at stwcx.xyz> wrote:
>>> From: "Milton D. Miller II" <miltonm at us.ibm.com>
>>>
>>> Changing the name of a device with dev_set_name after device_add
>>> is not allowed.
>>>
>>> However, of_create_device takes a bus_id parameter that specifies
>>> the name of the platform device being added.
>>>
>>> This does mean the bus property has to be retrieved multiple times.
>>> Creation is skiped if there is trouble retrieving or formatting
>>> the name.
>
> Spell checking this pointed out skipped above is missing a p in the
> commit message.
>
>>
>> Ok, this approach looks alright.
>>
>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>>> index 5488233..ccb438f 100644
>>>         for_each_child_of_node(pdev->dev.of_node, np) {
>>> -               of_platform_device_create(np, NULL, &pdev->dev);
>>> +               int ret;
>>> +               u32 bus_num;
>>> +               char bus_id[sizeof("i2c-12345")];
>>
>> Clever, but I don't think it's kernel style. A constant will do.
>
> git grep  'sizeof("'  | wc -l
>
> yields 235 occurrences, 94 with an additional \[ in front.  Including
> well maintained sections of the kernel like vsnprintf for uuid length
> and network code for formatting ip addresses.
>
> I admit the 5 digit bus number is a bit arbitrary, but I think the origin of
> a constant will be less clear than this code.

I was referring to doing sizeof("constant string").

>
>>> +
>>> +               ret = snprintf(bus_id, sizeof(bus_id), "i2c-%u", bus_num);
>>> +               if (ret >= sizeof(bus_id))
>>> +                       continue;
>>
>> You should check for a negative value as well.
>
> It's defined to return "the number of characters which would be written to the buffer",
> not sure how that can ever be < 0 other than the undefined overflow of the int type.
> vscnprintf doesn't consider a need to check for < 0.  The size_t input of the underlying
> vsnprintf is checked vs INT_MAX and returns 0 in that case.
>
> Can you point to other kernel code that checks printf family for returns < 0 ?

I was thinking of the userspace man page which suggests snprintf
returns a negative value on error. You're right; the kernel
implementation doesn't ever do this.