| Message ID | 8cb69f245311a348164b0b5ca3dbc59386746035.1712520052.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Daniel Lezcano |
| Headers | show |
| Series | [1/2] thermal/drivers/mediatek/lvts_thermal: Make debugfs related fields more consistent | expand |
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
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);
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 <christophe.jaillet@wanadoo.fr> --- Compile tested-only --- drivers/thermal/mediatek/lvts_thermal.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)