Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 3/5] thermal: core: Remove old uapi generic netlink
To:     Amit Kucheria <amit.kucheria@linaro.org>
Cc:     Zhang Rui <rui.zhang@intel.com>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        Ram Chandrasekar <rkumbako@codeaurora.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <20200625144509.17918-1-daniel.lezcano@linaro.org>
 <20200625144509.17918-3-daniel.lezcano@linaro.org>
 <CAP245DUMjTQr2vKirZ+FxEYWC=VQ_k+OegxQgXcKDU8ThWuCsQ@mail.gmail.com>
 <0fe6837f-9b44-4578-23f2-3e4932d01122@linaro.org>
 <CAP245DUG-OsSD-_CucMMQ26HpzjJhn0emfq_go923NsDq6RqOg@mail.gmail.com>
From:   Daniel Lezcano <daniel.lezcano@linaro.org>
Message-ID: <c664d247-7f9b-603f-c318-48e534aedfc9@linaro.org>
Date:   Wed, 1 Jul 2020 11:45:42 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.8.0
MIME-Version: 1.0
In-Reply-To: <CAP245DUG-OsSD-_CucMMQ26HpzjJhn0emfq_go923NsDq6RqOg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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:
>>>>
>>>> In order to set the scene for the new generic netlink thermal
>>>> management and notifications, let remove the old dead code remaining
>>>
>>> s/management/management api/
>>>
>>> s/let/let's/
>>>
>>>> in the uapi headers.
>>>>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>> ---
>>>>  include/linux/thermal.h      |  5 -----
>>>>  include/uapi/linux/thermal.h | 12 +-----------
>>>>  2 files changed, 1 insertion(+), 16 deletions(-)
>>>>
>>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>>> index faf7ad031e42..fc93a6348082 100644
>>>> --- a/include/linux/thermal.h
>>>> +++ b/include/linux/thermal.h
>>>> @@ -302,11 +302,6 @@ struct thermal_zone_params {
>>>>         int offset;
>>>>  };
>>>>
>>>> -struct thermal_genl_event {
>>>> -       u32 orig;
>>>> -       enum events event;
>>>> -};
>>>> -
>>>>  /**
>>>>   * struct thermal_zone_of_device_ops - scallbacks for handling DT based zones
>>>>   *
>>>> diff --git a/include/uapi/linux/thermal.h b/include/uapi/linux/thermal.h
>>>> index 96218378dda8..22df67ed9e9c 100644
>>>> --- a/include/uapi/linux/thermal.h
>>>> +++ b/include/uapi/linux/thermal.h
>>>> @@ -6,21 +6,12 @@
>>>>
>>>>  /* 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.


-- 
<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