The debugfs code is only generated if CONFIG_MTK_LVTS_THERMAL_DEBUGFS is
defined.
So 'dom_dentry' should be included in the 'struct lvts_domain' with the
same condition, instead of CONFIG_DEBUG_FS.
Signed-off-by: Christophe JAILLET <[email protected]>
---
Compile tested-only
---
drivers/thermal/mediatek/lvts_thermal.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
index fd4bd650c77a..3003dc350766 100644
--- a/drivers/thermal/mediatek/lvts_thermal.c
+++ b/drivers/thermal/mediatek/lvts_thermal.c
@@ -150,7 +150,7 @@ struct lvts_domain {
void __iomem *base;
size_t calib_len;
u8 *calib;
-#ifdef CONFIG_DEBUG_FS
+#ifdef CONFIG_MTK_LVTS_THERMAL_DEBUGFS
struct dentry *dom_dentry;
#endif
};
--
2.44.0
lvts_debugfs_init() is called with lvts_td->num_lvts_ctrl being 2, 3 or 4.
sizeof(struct debugfs_regset32) is small. 32 byres on a x86_64.
So, given the overhead of devm_kzalloc(), it is better to allocate all
needed regset at once.
The overhead of devm_kzalloc() is 40 bytes on a x86_64, and because of
rounding in memory allocation, it leads to:
2 * (32 + 40) = 2 * 72 --> 2 96 bytes allocations for a total of 192 bytes
3 * (32 + 40) = 3 * 72 --> 3 96 bytes allocations for a total of 288 bytes
4 * (32 + 40) = 4 * 72 --> 4 96 bytes allocations for a total of 384 bytes
using a single devm_kcalloc():
2 * 32 + 40 = 104 --> 1 allocation for a total of 128
3 * 32 + 40 = 136 --> 1 allocation for a total of 192
4 * 32 + 40 = 168 --> 1 allocation for a total of 192
So, this saves both a few bytes and reduce memory fragmentation.
Signed-off-by: Christophe JAILLET <[email protected]>
---
Compile tested-only
---
drivers/thermal/mediatek/lvts_thermal.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
index 3003dc350766..b133f731c5ba 100644
--- a/drivers/thermal/mediatek/lvts_thermal.c
+++ b/drivers/thermal/mediatek/lvts_thermal.c
@@ -204,7 +204,7 @@ static const struct debugfs_reg32 lvts_regs[] = {
static int lvts_debugfs_init(struct device *dev, struct lvts_domain *lvts_td)
{
- struct debugfs_regset32 *regset;
+ struct debugfs_regset32 *regsets;
struct lvts_ctrl *lvts_ctrl;
struct dentry *dentry;
char name[64];
@@ -214,8 +214,14 @@ static int lvts_debugfs_init(struct device *dev, struct lvts_domain *lvts_td)
if (IS_ERR(lvts_td->dom_dentry))
return 0;
+ regsets = devm_kcalloc(dev, lvts_td->num_lvts_ctrl,
+ sizeof(*regsets), GFP_KERNEL);
+ if (!regsets)
+ return 0;
+
for (i = 0; i < lvts_td->num_lvts_ctrl; i++) {
+ struct debugfs_regset32 *regset = ®sets[i];
lvts_ctrl = &lvts_td->lvts_ctrl[i];
sprintf(name, "controller%d", i);
@@ -223,10 +229,6 @@ static int lvts_debugfs_init(struct device *dev, struct lvts_domain *lvts_td)
if (IS_ERR(dentry))
continue;
- regset = devm_kzalloc(dev, sizeof(*regset), GFP_KERNEL);
- if (!regset)
- continue;
-
regset->base = lvts_ctrl->base;
regset->regs = lvts_regs;
regset->nregs = ARRAY_SIZE(lvts_regs);
--
2.44.0
On Sun, Apr 07, 2024 at 10:01:49PM +0200, Christophe JAILLET wrote:
> diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
> index 3003dc350766..b133f731c5ba 100644
> --- a/drivers/thermal/mediatek/lvts_thermal.c
> +++ b/drivers/thermal/mediatek/lvts_thermal.c
> @@ -204,7 +204,7 @@ static const struct debugfs_reg32 lvts_regs[] = {
>
> static int lvts_debugfs_init(struct device *dev, struct lvts_domain *lvts_td)
> {
> - struct debugfs_regset32 *regset;
> + struct debugfs_regset32 *regsets;
> struct lvts_ctrl *lvts_ctrl;
> struct dentry *dentry;
> char name[64];
> @@ -214,8 +214,14 @@ static int lvts_debugfs_init(struct device *dev, struct lvts_domain *lvts_td)
> if (IS_ERR(lvts_td->dom_dentry))
> return 0;
>
> + regsets = devm_kcalloc(dev, lvts_td->num_lvts_ctrl,
> + sizeof(*regsets), GFP_KERNEL);
> + if (!regsets)
> + return 0;
I understand that this preserved the behavior from the original code,
but the original code was wrong. This should return -ENOMEM.
> +
> for (i = 0; i < lvts_td->num_lvts_ctrl; i++) {
>
> + struct debugfs_regset32 *regset = ®sets[i];
> lvts_ctrl = &lvts_td->lvts_ctrl[i];
The blank line should come after the declaration.
regards,
dan carpenter
Le 08/04/2024 à 10:09, Dan Carpenter a écrit :
> On Sun, Apr 07, 2024 at 10:01:49PM +0200, Christophe JAILLET wrote:
>> diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
>> index 3003dc350766..b133f731c5ba 100644
>> --- a/drivers/thermal/mediatek/lvts_thermal.c
>> +++ b/drivers/thermal/mediatek/lvts_thermal.c
>> @@ -204,7 +204,7 @@ static const struct debugfs_reg32 lvts_regs[] = {
>>
>> static int lvts_debugfs_init(struct device *dev, struct lvts_domain *lvts_td)
>> {
>> - struct debugfs_regset32 *regset;
>> + struct debugfs_regset32 *regsets;
>> struct lvts_ctrl *lvts_ctrl;
>> struct dentry *dentry;
>> char name[64];
>> @@ -214,8 +214,14 @@ static int lvts_debugfs_init(struct device *dev, struct lvts_domain *lvts_td)
>> if (IS_ERR(lvts_td->dom_dentry))
>> return 0;
>>
>> + regsets = devm_kcalloc(dev, lvts_td->num_lvts_ctrl,
>> + sizeof(*regsets), GFP_KERNEL);
>> + if (!regsets)
>> + return 0;
>
> I understand that this preserved the behavior from the original code,
> but the original code was wrong. This should return -ENOMEM.
Hi Dan,
I don't agree.
For me, this memory allocation is of the same type as all debugfs
functions that we ignore the error code.
If it fails, it is not a reason good enough to have the probe fail.
(anyway, if we are short on memory at this point other errors will
likely occur)
>
>> +
>> for (i = 0; i < lvts_td->num_lvts_ctrl; i++) {
>>
>> + struct debugfs_regset32 *regset = ®sets[i];
>> lvts_ctrl = &lvts_td->lvts_ctrl[i];
>
> The blank line should come after the declaration.
The blank line was already there, and in this file, it looks like the
preferred style (even if not completely consistent)
Let see if there is some comment about 0 or -ENOMEM in case of memory
allocation error, and if needed, I'll repost without the blank line.
This patch being a really tiny tiny tiny improvement (IMHO), so it may
also just be ignored.
CJ
>
> regards,
> dan carpenter
>
>
>
On Mon, Apr 08, 2024 at 08:41:26PM +0200, Christophe JAILLET wrote:
> Le 08/04/2024 ? 10:09, Dan Carpenter a ?crit?:
> > On Sun, Apr 07, 2024 at 10:01:49PM +0200, Christophe JAILLET wrote:
> > > diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
> > > index 3003dc350766..b133f731c5ba 100644
> > > --- a/drivers/thermal/mediatek/lvts_thermal.c
> > > +++ b/drivers/thermal/mediatek/lvts_thermal.c
> > > @@ -204,7 +204,7 @@ static const struct debugfs_reg32 lvts_regs[] = {
> > > static int lvts_debugfs_init(struct device *dev, struct lvts_domain *lvts_td)
> > > {
> > > - struct debugfs_regset32 *regset;
> > > + struct debugfs_regset32 *regsets;
> > > struct lvts_ctrl *lvts_ctrl;
> > > struct dentry *dentry;
> > > char name[64];
> > > @@ -214,8 +214,14 @@ static int lvts_debugfs_init(struct device *dev, struct lvts_domain *lvts_td)
> > > if (IS_ERR(lvts_td->dom_dentry))
> > > return 0;
> > > + regsets = devm_kcalloc(dev, lvts_td->num_lvts_ctrl,
> > > + sizeof(*regsets), GFP_KERNEL);
> > > + if (!regsets)
> > > + return 0;
> >
> > I understand that this preserved the behavior from the original code,
> > but the original code was wrong. This should return -ENOMEM.
>
> Hi Dan,
> I don't agree.
>
> For me, this memory allocation is of the same type as all debugfs functions
> that we ignore the error code.
>
> If it fails, it is not a reason good enough to have the probe fail. (anyway,
> if we are short on memory at this point other errors will likely occur)
>
Huh. It's an interesting point. Fair enough.
> >
> > > +
> > > for (i = 0; i < lvts_td->num_lvts_ctrl; i++) {
> > > + struct debugfs_regset32 *regset = ®sets[i];
> > > lvts_ctrl = &lvts_td->lvts_ctrl[i];
> >
> > The blank line should come after the declaration.
>
> The blank line was already there, and in this file, it looks like the
> preferred style (even if not completely consistent)
>
> Let see if there is some comment about 0 or -ENOMEM in case of memory
> allocation error, and if needed, I'll repost without the blank line.
>
There is supposed to be a blank line after declarations though so I
think if you re-run checkpatch.pl -f on the file there is a checkpatch
warning now.
regards,
dan carpenter