Re: [PATCH v2 3/5] thermal: core: Remove old uapi generic netlink

Re: [PATCH v2 3/5] thermal: core: Remove old uapi generic netlink

[Amit Kucheria]

On Wed, Jul 1, 2020 at 3:15 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 01/07/2020 11:33, Amit Kucheria wrote:
> > On Wed, Jul 1, 2020 at 2:56 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 30/06/2020 13:47, Amit Kucheria wrote:
> >>> On Thu, Jun 25, 2020 at 8:15 PM Daniel Lezcano
> >>> <daniel.lezcano@linaro.org> wrote:
> >>>>

> >>>>  /* Adding event notification support elements */
> >>>>  #define THERMAL_GENL_FAMILY_NAME                "thermal_event"
> >>>> -#define THERMAL_GENL_VERSION                    0x01
> >>>> +#define THERMAL_GENL_VERSION                    0x02
> >>>
> >>> This hunk should be removed since you set version back to 1 in the
> >>> next patch and we don't actually intend to bump the version yet.
> >>
> >> Well, I've been very strict here for git-bisecting.
> >>
> >> I move to V2 because of the removal, but when adding the new genetlink
> >> code, the family name changed, so we returned back to the V1 as it is a
> >> new genetlink thermal brand.
> >
> > I don't understand the move to v2 for an empty skeleton UAPI. For the
> > purposes of bisection, couldn't you just remove all the v1 UAPI (w/o
> > bumping to v2) and then add a new UAPI in the next patch?
> >
> >> The name is change because it is no longer event based but also sampling
> >> and commands.
> >
> > In this case, just to avoid any confusion, the new UAPI could be v2
> > making the transition clear in case of bisection.
> >
> > I'm afraid the v1->v2->v1 is a bit more confusing.
>
> Let me elaborate a bit:
>
> Why there is this patch ?
> - By removing this code first, the next patch will just contain
> additions, I thought it would be clearer
>
> Why increase the version here ?
> - Code must continue to compile and as the 'thermal_event' family is now
> different from V1, the version is changed
>
> Why the version goes to V1 in the next patch ?
> - The family name is changed as it is not doing event only, so it is a
> new netlink thermal protocol and we begin at V1
>
> So the main reason of this patch is to be very strict in the iteration
> changes. May be it is too much, in this case I can merge this patch with
> 4/5, the old netlink protocol removal will be lost in the addition of
> the new protocol. I'm fine with that if you think it is simpler.

Considering that there are no users of v1 currently, it feels a bit
over engineered, IMHO.

Also, the new UAPI doesn't need to begin at v1. Just having it start
at v2 will avoid this confusion, no?

Re: [PATCH v2 3/5] thermal: core: Remove old uapi generic netlink

[Daniel Lezcano]

On 01/07/2020 14:10, Amit Kucheria wrote:
> On Wed, Jul 1, 2020 at 3:15 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> On 01/07/2020 11:33, Amit Kucheria wrote:
>>> On Wed, Jul 1, 2020 at 2:56 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> On 30/06/2020 13:47, Amit Kucheria wrote:
>>>>> On Thu, Jun 25, 2020 at 8:15 PM Daniel Lezcano
>>>>> <daniel.lezcano@linaro.org> wrote:
>>>>>>
> 
>>>>>>  /* Adding event notification support elements */
>>>>>>  #define THERMAL_GENL_FAMILY_NAME                "thermal_event"
>>>>>> -#define THERMAL_GENL_VERSION                    0x01
>>>>>> +#define THERMAL_GENL_VERSION                    0x02
>>>>>
>>>>> This hunk should be removed since you set version back to 1 in the
>>>>> next patch and we don't actually intend to bump the version yet.
>>>>
>>>> Well, I've been very strict here for git-bisecting.
>>>>
>>>> I move to V2 because of the removal, but when adding the new genetlink
>>>> code, the family name changed, so we returned back to the V1 as it is a
>>>> new genetlink thermal brand.
>>>
>>> I don't understand the move to v2 for an empty skeleton UAPI. For the
>>> purposes of bisection, couldn't you just remove all the v1 UAPI (w/o
>>> bumping to v2) and then add a new UAPI in the next patch?
>>>
>>>> The name is change because it is no longer event based but also sampling
>>>> and commands.
>>>
>>> In this case, just to avoid any confusion, the new UAPI could be v2
>>> making the transition clear in case of bisection.
>>>
>>> I'm afraid the v1->v2->v1 is a bit more confusing.
>>
>> Let me elaborate a bit:
>>
>> Why there is this patch ?
>> - By removing this code first, the next patch will just contain
>> additions, I thought it would be clearer
>>
>> Why increase the version here ?
>> - Code must continue to compile and as the 'thermal_event' family is now
>> different from V1, the version is changed
>>
>> Why the version goes to V1 in the next patch ?
>> - The family name is changed as it is not doing event only, so it is a
>> new netlink thermal protocol and we begin at V1
>>
>> So the main reason of this patch is to be very strict in the iteration
>> changes. May be it is too much, in this case I can merge this patch with
>> 4/5, the old netlink protocol removal will be lost in the addition of
>> the new protocol. I'm fine with that if you think it is simpler.
> 
> Considering that there are no users of v1 currently, it feels a bit
> over engineered, IMHO.
> 
> Also, the new UAPI doesn't need to begin at v1. Just having it start
> at v2 will avoid this confusion, no?

Ok, I will merge both patches but I will keep the V1 because the netlink
protocol is a new one.


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog