[PATCH 6.6] thermal/of: Fix mask mismatch when no trips subnode

[PATCH 6.6] thermal/of: Fix mask mismatch when no trips subnode

[Hsin-Te Yuan]

After commit 725f31f300e3 ("thermal/of: support thermal zones w/o trips
subnode") was backported on 6.6 stable branch as commit d3304dbc2d5f
("thermal/of: support thermal zones w/o trips subnode"), thermal zones
w/o trips subnode still fail to register since `mask` argument is not
set correctly. When number of trips subnode is 0, `mask` must be 0 to
pass the check in `thermal_zone_device_register_with_trips()`.

Set `mask` to 0 when there's no trips subnode.

Signed-off-by: Hsin-Te Yuan <yuanhsinte@chromium.org>
---
 drivers/thermal/thermal_of.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
index 0f520cf923a1e684411a3077ad283551395eec11..97aeb869abf5179dfa512dd744725121ec7fd0d9 100644
--- a/drivers/thermal/thermal_of.c
+++ b/drivers/thermal/thermal_of.c
@@ -514,7 +514,7 @@ static struct thermal_zone_device *thermal_of_zone_register(struct device_node *
 	of_ops->bind = thermal_of_bind;
 	of_ops->unbind = thermal_of_unbind;
 
-	mask = GENMASK_ULL((ntrips) - 1, 0);
+	mask = ntrips ? GENMASK_ULL((ntrips) - 1, 0) : 0;
 
 	tz = thermal_zone_device_register_with_trips(np->name, trips, ntrips,
 						     mask, data, of_ops, &tzp,

---
base-commit: a5df3a702b2cba82bcfb066fa9f5e4a349c33924
change-id: 20250707-trip-point-73dae9fd9c74

Best regards,
-- 
Hsin-Te Yuan <yuanhsinte@chromium.org>

Re: [PATCH 6.6] thermal/of: Fix mask mismatch when no trips subnode

[Rafael J. Wysocki]

On Mon, Jul 7, 2025 at 12:27 PM Hsin-Te Yuan <yuanhsinte@chromium.org> wrote:
>
> After commit 725f31f300e3 ("thermal/of: support thermal zones w/o trips
> subnode") was backported on 6.6 stable branch as commit d3304dbc2d5f
> ("thermal/of: support thermal zones w/o trips subnode"), thermal zones
> w/o trips subnode still fail to register since `mask` argument is not
> set correctly. When number of trips subnode is 0, `mask` must be 0 to
> pass the check in `thermal_zone_device_register_with_trips()`.
>
> Set `mask` to 0 when there's no trips subnode.
>
> Signed-off-by: Hsin-Te Yuan <yuanhsinte@chromium.org>
> ---
>  drivers/thermal/thermal_of.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> index 0f520cf923a1e684411a3077ad283551395eec11..97aeb869abf5179dfa512dd744725121ec7fd0d9 100644
> --- a/drivers/thermal/thermal_of.c
> +++ b/drivers/thermal/thermal_of.c
> @@ -514,7 +514,7 @@ static struct thermal_zone_device *thermal_of_zone_register(struct device_node *
>         of_ops->bind = thermal_of_bind;
>         of_ops->unbind = thermal_of_unbind;
>
> -       mask = GENMASK_ULL((ntrips) - 1, 0);
> +       mask = ntrips ? GENMASK_ULL((ntrips) - 1, 0) : 0;
>
>         tz = thermal_zone_device_register_with_trips(np->name, trips, ntrips,
>                                                      mask, data, of_ops, &tzp,
>
> ---

If this issue is present in the mainline, it is not necessary to
mention "stable" in the changelog.

Just post a patch against the mainline with an appropriate Fixes: tag.

Thanks!

Re: [PATCH 6.6] thermal/of: Fix mask mismatch when no trips subnode

[Hsin-Te Yuan]

On Tue, Jul 8, 2025 at 12:57 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Jul 7, 2025 at 12:27 PM Hsin-Te Yuan <yuanhsinte@chromium.org> wrote:
> >
> > After commit 725f31f300e3 ("thermal/of: support thermal zones w/o trips
> > subnode") was backported on 6.6 stable branch as commit d3304dbc2d5f
> > ("thermal/of: support thermal zones w/o trips subnode"), thermal zones
> > w/o trips subnode still fail to register since `mask` argument is not
> > set correctly. When number of trips subnode is 0, `mask` must be 0 to
> > pass the check in `thermal_zone_device_register_with_trips()`.
> >
> > Set `mask` to 0 when there's no trips subnode.
> >
> > Signed-off-by: Hsin-Te Yuan <yuanhsinte@chromium.org>
> > ---
> >  drivers/thermal/thermal_of.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> > index 0f520cf923a1e684411a3077ad283551395eec11..97aeb869abf5179dfa512dd744725121ec7fd0d9 100644
> > --- a/drivers/thermal/thermal_of.c
> > +++ b/drivers/thermal/thermal_of.c
> > @@ -514,7 +514,7 @@ static struct thermal_zone_device *thermal_of_zone_register(struct device_node *
> >         of_ops->bind = thermal_of_bind;
> >         of_ops->unbind = thermal_of_unbind;
> >
> > -       mask = GENMASK_ULL((ntrips) - 1, 0);
> > +       mask = ntrips ? GENMASK_ULL((ntrips) - 1, 0) : 0;
> >
> >         tz = thermal_zone_device_register_with_trips(np->name, trips, ntrips,
> >                                                      mask, data, of_ops, &tzp,
> >
> > ---
>
> If this issue is present in the mainline, it is not necessary to
> mention "stable" in the changelog.
>
> Just post a patch against the mainline with an appropriate Fixes: tag.
>
> Thanks!
`mask` has been removed from the mainline, so this patch is only
applicable on old branches.

Re: [PATCH 6.6] thermal/of: Fix mask mismatch when no trips subnode

[Rafael J. Wysocki]

On Tue, Jul 8, 2025 at 8:41 AM Hsin-Te Yuan <yuanhsinte@chromium.org> wrote:
>
> On Tue, Jul 8, 2025 at 12:57 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Mon, Jul 7, 2025 at 12:27 PM Hsin-Te Yuan <yuanhsinte@chromium.org> wrote:
> > >
> > > After commit 725f31f300e3 ("thermal/of: support thermal zones w/o trips
> > > subnode") was backported on 6.6 stable branch as commit d3304dbc2d5f
> > > ("thermal/of: support thermal zones w/o trips subnode"), thermal zones
> > > w/o trips subnode still fail to register since `mask` argument is not
> > > set correctly. When number of trips subnode is 0, `mask` must be 0 to
> > > pass the check in `thermal_zone_device_register_with_trips()`.
> > >
> > > Set `mask` to 0 when there's no trips subnode.
> > >
> > > Signed-off-by: Hsin-Te Yuan <yuanhsinte@chromium.org>
> > > ---
> > >  drivers/thermal/thermal_of.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> > > index 0f520cf923a1e684411a3077ad283551395eec11..97aeb869abf5179dfa512dd744725121ec7fd0d9 100644
> > > --- a/drivers/thermal/thermal_of.c
> > > +++ b/drivers/thermal/thermal_of.c
> > > @@ -514,7 +514,7 @@ static struct thermal_zone_device *thermal_of_zone_register(struct device_node *
> > >         of_ops->bind = thermal_of_bind;
> > >         of_ops->unbind = thermal_of_unbind;
> > >
> > > -       mask = GENMASK_ULL((ntrips) - 1, 0);
> > > +       mask = ntrips ? GENMASK_ULL((ntrips) - 1, 0) : 0;
> > >
> > >         tz = thermal_zone_device_register_with_trips(np->name, trips, ntrips,
> > >                                                      mask, data, of_ops, &tzp,
> > >
> > > ---
> >
> > If this issue is present in the mainline, it is not necessary to
> > mention "stable" in the changelog.
> >
> > Just post a patch against the mainline with an appropriate Fixes: tag.
> >
> > Thanks!
> `mask` has been removed from the mainline, so this patch is only
> applicable on old branches.

So the way to go would be to follow the mainline in those branches.

Re: [PATCH 6.6] thermal/of: Fix mask mismatch when no trips subnode

[Greg KH]

On Mon, Jul 07, 2025 at 06:27:10PM +0800, Hsin-Te Yuan wrote:
> After commit 725f31f300e3 ("thermal/of: support thermal zones w/o trips
> subnode") was backported on 6.6 stable branch as commit d3304dbc2d5f
> ("thermal/of: support thermal zones w/o trips subnode"), thermal zones
> w/o trips subnode still fail to register since `mask` argument is not
> set correctly. When number of trips subnode is 0, `mask` must be 0 to
> pass the check in `thermal_zone_device_register_with_trips()`.
> 
> Set `mask` to 0 when there's no trips subnode.
> 
> Signed-off-by: Hsin-Te Yuan <yuanhsinte@chromium.org>
> ---
>  drivers/thermal/thermal_of.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> index 0f520cf923a1e684411a3077ad283551395eec11..97aeb869abf5179dfa512dd744725121ec7fd0d9 100644
> --- a/drivers/thermal/thermal_of.c
> +++ b/drivers/thermal/thermal_of.c
> @@ -514,7 +514,7 @@ static struct thermal_zone_device *thermal_of_zone_register(struct device_node *
>  	of_ops->bind = thermal_of_bind;
>  	of_ops->unbind = thermal_of_unbind;
>  
> -	mask = GENMASK_ULL((ntrips) - 1, 0);
> +	mask = ntrips ? GENMASK_ULL((ntrips) - 1, 0) : 0;

Meta-comment, I hate ? : lines in C, especially when they are not
needed, like here.  Spell this out, with a real if statement please, so
that we can read and easily understand what is going on.

That being said, I agree with Rafael, let's do whatever is in mainline
instead.  Fix it the same way it was fixed there by backporting the
relevant commits.

thanks,

greg k-h

Re: [PATCH 6.6] thermal/of: Fix mask mismatch when no trips subnode

[Hsin-Te Yuan]

On Thu, Jul 10, 2025 at 9:33 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jul 07, 2025 at 06:27:10PM +0800, Hsin-Te Yuan wrote:
> > After commit 725f31f300e3 ("thermal/of: support thermal zones w/o trips
> > subnode") was backported on 6.6 stable branch as commit d3304dbc2d5f
> > ("thermal/of: support thermal zones w/o trips subnode"), thermal zones
> > w/o trips subnode still fail to register since `mask` argument is not
> > set correctly. When number of trips subnode is 0, `mask` must be 0 to
> > pass the check in `thermal_zone_device_register_with_trips()`.
> >
> > Set `mask` to 0 when there's no trips subnode.
> >
> > Signed-off-by: Hsin-Te Yuan <yuanhsinte@chromium.org>
> > ---
> >  drivers/thermal/thermal_of.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> > index 0f520cf923a1e684411a3077ad283551395eec11..97aeb869abf5179dfa512dd744725121ec7fd0d9 100644
> > --- a/drivers/thermal/thermal_of.c
> > +++ b/drivers/thermal/thermal_of.c
> > @@ -514,7 +514,7 @@ static struct thermal_zone_device *thermal_of_zone_register(struct device_node *
> >       of_ops->bind = thermal_of_bind;
> >       of_ops->unbind = thermal_of_unbind;
> >
> > -     mask = GENMASK_ULL((ntrips) - 1, 0);
> > +     mask = ntrips ? GENMASK_ULL((ntrips) - 1, 0) : 0;
>
> Meta-comment, I hate ? : lines in C, especially when they are not
> needed, like here.  Spell this out, with a real if statement please, so
> that we can read and easily understand what is going on.
>
I will change this in v2 if we end up going with this solution.

> That being said, I agree with Rafael, let's do whatever is in mainline
> instead.  Fix it the same way it was fixed there by backporting the
> relevant commits.
>
> thanks,
>
> greg k-h

`mask` is removed in 83c2d444ed9d ("thermal: of: Set
THERMAL_TRIP_FLAG_RW_TEMP directly"), which needs 5340f7647294
("thermal: core: Add flags to struct thermal_trip"). I think it's
beyond a fix to introduce this. Also, there were several conflicts
when I tried to cherry-pick 5340f7647294. Compared to a simple
solution like setting `mask` to 0, I don't think it's worthwhile and
safe to cherry-pick all the dependencies.

Regards,
Hsin-Te

Re: [PATCH 6.6] thermal/of: Fix mask mismatch when no trips subnode

[Greg KH]

On Mon, Jul 14, 2025 at 08:36:29PM +0800, Hsin-Te Yuan wrote:
> On Thu, Jul 10, 2025 at 9:33 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Jul 07, 2025 at 06:27:10PM +0800, Hsin-Te Yuan wrote:
> > > After commit 725f31f300e3 ("thermal/of: support thermal zones w/o trips
> > > subnode") was backported on 6.6 stable branch as commit d3304dbc2d5f
> > > ("thermal/of: support thermal zones w/o trips subnode"), thermal zones
> > > w/o trips subnode still fail to register since `mask` argument is not
> > > set correctly. When number of trips subnode is 0, `mask` must be 0 to
> > > pass the check in `thermal_zone_device_register_with_trips()`.
> > >
> > > Set `mask` to 0 when there's no trips subnode.
> > >
> > > Signed-off-by: Hsin-Te Yuan <yuanhsinte@chromium.org>
> > > ---
> > >  drivers/thermal/thermal_of.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> > > index 0f520cf923a1e684411a3077ad283551395eec11..97aeb869abf5179dfa512dd744725121ec7fd0d9 100644
> > > --- a/drivers/thermal/thermal_of.c
> > > +++ b/drivers/thermal/thermal_of.c
> > > @@ -514,7 +514,7 @@ static struct thermal_zone_device *thermal_of_zone_register(struct device_node *
> > >       of_ops->bind = thermal_of_bind;
> > >       of_ops->unbind = thermal_of_unbind;
> > >
> > > -     mask = GENMASK_ULL((ntrips) - 1, 0);
> > > +     mask = ntrips ? GENMASK_ULL((ntrips) - 1, 0) : 0;
> >
> > Meta-comment, I hate ? : lines in C, especially when they are not
> > needed, like here.  Spell this out, with a real if statement please, so
> > that we can read and easily understand what is going on.
> >
> I will change this in v2 if we end up going with this solution.
> 
> > That being said, I agree with Rafael, let's do whatever is in mainline
> > instead.  Fix it the same way it was fixed there by backporting the
> > relevant commits.
> >
> > thanks,
> >
> > greg k-h
> 
> `mask` is removed in 83c2d444ed9d ("thermal: of: Set
> THERMAL_TRIP_FLAG_RW_TEMP directly"), which needs 5340f7647294
> ("thermal: core: Add flags to struct thermal_trip"). I think it's
> beyond a fix to introduce this. Also, there were several conflicts
> when I tried to cherry-pick 5340f7647294. Compared to a simple
> solution like setting `mask` to 0, I don't think it's worthwhile and
> safe to cherry-pick all the dependencies.

Remember, every patch you add to the tree that is NOT upstream, will
almost always cause problems later on, if not immediately (we have a
lousy track record of one-off-patches.)  Also this prevents future
upstream changes from being able to be applied to the tree.

And as you will now be responsible for maintaining this for the next 3-4
years, do whatever possible to make it easy to keep alive properly.

thanks,

greg k-h