Messages in this thread | | | From | Amit Kucheria <> | Date | Wed, 1 Jul 2020 17:40:28 +0530 | Subject | Re: [PATCH v2 3/5] thermal: core: Remove old uapi generic netlink |
| |
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?
| |