Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Wed, 27 Jul 2022 16:20:12 +0200 | Subject | Re: [PATCH V2 1/1] thermal/sysfs: Clear cooling_device_stats_attr_group before initialized |
| |
On Wed, Jul 27, 2022 at 10:17 AM Di Shen <cindygm567@gmail.com> wrote: > > Hi Rafael, > I have tested this patch on our platform, and it works. Later, I will > send the patch v3.
Well, no need. I'll use the one that you've just tested.
Thanks!
> On Tue, Jul 26, 2022 at 3:39 PM Di Shen <cindygm567@gmail.com> wrote: > > > > On Sat, Jul 23, 2022 at 2:42 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > > > > On Friday, July 22, 2022 7:18:42 PM CEST Rafael J. Wysocki wrote: > > > > On Fri, Jul 22, 2022 at 10:44 AM Di Shen <di.shen@unisoc.com> wrote: > > > > > > > > > > There's a space allocated for cooling_device_stats_attr_group > > > > > within cooling_device_attr_groups. This space is shared by all > > > > > cooling devices. > > > > > > > > That's correct. > > > > > > > > > If the stats structure of one cooling device successfully > > > > > creates stats sysfs. After that, another cooling device fails > > > > > to get max_states in cooling_device_stats_setup(). It can > > > > > return directly without initializing the stats structure, but > > > > > the cooling_device_stats_attr_group is still the attribute > > > > > group of the last cooling device. > > > > > > > > I cannot parse the above, sorry. > > > > > > > > For example, how can a "stats structure of one cooling device" create > > > > anything? As a data structure, it is a passive entity, so it doesn't > > > > carry out any actions. > > > > > > > > I think (but I am not sure) that you are referring to the error code > > > > path in which the ->get_max_state() callback fails for a cooling > > > > device after cooling_device_stats_setup() has completed successfully > > > > for another one. > > > > > > > > > At this time, read or write stats sysfs nodes can cause kernel > > > > > crash. Like the following, kernel crashed when > > > > > 'cat time_in_state_ms'. > > > > > > > > > > [<5baac8d4>] panic+0x1b4/0x3c8 > > > > > [<9d287b0f>] arm_notify_die+0x0/0x78 > > > > > [<094fc22c>] __do_kernel_fault+0x94/0xa4 > > > > > [<3b4b69a4>] do_page_fault+0xd4/0x364 > > > > > [<23793e7a>] do_translation_fault+0x38/0xc0 > > > > > [<6e5cc52a>] do_DataAbort+0x4c/0xd0 > > > > > [<a28c16b8>] __dabt_svc+0x5c/0xa0 > > > > > [<747516ae>] _raw_spin_lock+0x20/0x60 > > > > > [<9a9e4cd4>] time_in_state_ms_show+0x28/0x148 > > > > > [<cb78325e>] dev_attr_show+0x38/0x64 > > > > > [<aea3e364>] sysfs_kf_seq_show+0x8c/0xf0 > > > > > [<c0a843ab>] seq_read+0x244/0x620 > > > > > [<b316b374>] vfs_read+0xd8/0x218 > > > > > [<3aebf5fa>] sys_read+0x80/0xe4 > > > > > [<7cf100f5>] ret_fast_syscall+0x0/0x28 > > > > > [<08cbe22f>] 0xbe8c1198 > > > > > > > > > > stats sysfs: > > > > > phone:/sys/class/thermal/cooling_device2/stats # ls > > > > > reset time_in_state_ms total_trans trans_table > > > > > > > > > > The same as cat total_trans, trans_table, and echo reset. > > > > > > So does the (untested) patch below work too? > > > > > > > Yes, I agree with you. I will test it on our platform and give > > a reply later. Thanks. > > > > > --- > > > drivers/thermal/thermal_sysfs.c | 10 +++++++--- > > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > > > Index: linux-pm/drivers/thermal/thermal_sysfs.c > > > =================================================================== > > > --- linux-pm.orig/drivers/thermal/thermal_sysfs.c > > > +++ linux-pm/drivers/thermal/thermal_sysfs.c > > > @@ -813,12 +813,13 @@ static const struct attribute_group cool > > > > > > static void cooling_device_stats_setup(struct thermal_cooling_device *cdev) > > > { > > > + const struct attribute_group *stats_attr_group = NULL; > > > struct cooling_dev_stats *stats; > > > unsigned long states; > > > int var; > > > > > > if (cdev->ops->get_max_state(cdev, &states)) > > > - return; > > > + goto out; > > > > > > states++; /* Total number of states is highest state + 1 */ > > > > > > @@ -828,7 +829,7 @@ static void cooling_device_stats_setup(s > > > > > > stats = kzalloc(var, GFP_KERNEL); > > > if (!stats) > > > - return; > > > + goto out; > > > > > > stats->time_in_state = (ktime_t *)(stats + 1); > > > stats->trans_table = (unsigned int *)(stats->time_in_state + states); > > > @@ -838,9 +839,12 @@ static void cooling_device_stats_setup(s > > > > > > spin_lock_init(&stats->lock); > > > > > > + stats_attr_group = &cooling_device_stats_attr_group; > > > + > > > +out: > > > /* Fill the empty slot left in cooling_device_attr_groups */ > > > var = ARRAY_SIZE(cooling_device_attr_groups) - 2; > > > - cooling_device_attr_groups[var] = &cooling_device_stats_attr_group; > > > + cooling_device_attr_groups[var] = stats_attr_group; > > > } > > > > > > static void cooling_device_stats_destroy(struct thermal_cooling_device *cdev) > > > > > > > > > > > -- > > Best regards,
| |