* Re: [PATCH] fbdev: modedb: keep mode option buffer until parsing completes
From: Helge Deller @ 2026-06-09 16:07 UTC (permalink / raw)
To: Ruoyu Wang, Simona Vetter, Javier Martinez Canillas,
Thomas Zimmermann
Cc: linux-fbdev, dri-devel, linux-kernel
In-Reply-To: <20260609160028.5-1-ruoyuw560@gmail.com>
On 6/9/26 18:00, Ruoyu Wang wrote:
> When fb_find_mode() obtains the mode option from fb_get_options(),
> mode_option_buf owns the returned string and name points into that
> buffer. The done label frees mode_option_buf before the database
> fallback has finished using name in name_matches(), so the fallback can
> read freed memory.
>
> Move the free to a common exit path and convert the successful returns
> that can use mode_option_buf into jumps to that exit path.
There was another similiar patch already posted:
https://patchwork.kernel.org/project/linux-fbdev/patch/20260526091507.421730-1-islituo@gmail.com/
Do you want to check if the Scope-based kfree can be used here,
as suggested by me in that thread? It's at least much smaller than your patch...
Helge
>
> Fixes: 089d924d03d5 ("fbdev: Read video= option with fb_get_option() in modedb")
> Signed-off-by: Ruoyu Wang <ruoyuw560@gmail.com>
> ---
> drivers/video/fbdev/core/modedb.c | 41 ++++++++++++++++++++-----------
> 1 file changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/modedb.c b/drivers/video/fbdev/core/modedb.c
> index 703d0b7aec322..82f6ea38e1fb8 100644
> --- a/drivers/video/fbdev/core/modedb.c
> +++ b/drivers/video/fbdev/core/modedb.c
> @@ -627,7 +627,7 @@ int fb_find_mode(struct fb_var_screeninfo *var,
> unsigned int default_bpp)
> {
> char *mode_option_buf = NULL;
> - int i;
> + int i, ret;
>
> /* Set up defaults */
> if (!db) {
> @@ -724,10 +724,9 @@ int fb_find_mode(struct fb_var_screeninfo *var,
> res_specified = 1;
> }
> done:
> - kfree(mode_option_buf);
> if (cvt) {
> struct fb_videomode cvt_mode;
> - int ret;
> + int cvt_ret;
>
> DPRINTK("CVT mode %dx%d@%dHz%s%s%s\n", xres, yres,
> (refresh) ? refresh : 60,
> @@ -745,11 +744,12 @@ int fb_find_mode(struct fb_var_screeninfo *var,
> else
> cvt_mode.vmode &= ~FB_VMODE_INTERLACED;
>
> - ret = fb_find_mode_cvt(&cvt_mode, margins, rb);
> + cvt_ret = fb_find_mode_cvt(&cvt_mode, margins, rb);
>
> - if (!ret && !fb_try_mode(var, info, &cvt_mode, bpp)) {
> + if (!cvt_ret && !fb_try_mode(var, info, &cvt_mode, bpp)) {
> DPRINTK("modedb CVT: CVT mode ok\n");
> - return 1;
> + ret = 1;
> + goto out;
> }
>
> DPRINTK("CVT mode invalid, getting mode from database\n");
> @@ -793,8 +793,10 @@ int fb_find_mode(struct fb_var_screeninfo *var,
> if (!interlace_specified ||
> db_interlace == interlace)
> if (refresh_specified &&
> - db[i].refresh == refresh)
> - return 1;
> + db[i].refresh == refresh) {
> + ret = 1;
> + goto out;
> + }
>
> if (score < diff) {
> diff = score;
> @@ -804,7 +806,8 @@ int fb_find_mode(struct fb_var_screeninfo *var,
> }
> if (best != -1) {
> fb_try_mode(var, info, &db[best], bpp);
> - return (refresh_specified) ? 2 : 1;
> + ret = (refresh_specified) ? 2 : 1;
> + goto out;
> }
>
> diff = 2 * (xres + yres);
> @@ -831,21 +834,29 @@ int fb_find_mode(struct fb_var_screeninfo *var,
> }
> if (best != -1) {
> fb_try_mode(var, info, &db[best], bpp);
> - return 5;
> + ret = 5;
> + goto out;
> }
> }
>
> DPRINTK("Trying default video mode\n");
> - if (!fb_try_mode(var, info, default_mode, default_bpp))
> - return 3;
> + if (!fb_try_mode(var, info, default_mode, default_bpp)) {
> + ret = 3;
> + goto out;
> + }
>
> DPRINTK("Trying all modes\n");
> for (i = 0; i < dbsize; i++)
> - if (!fb_try_mode(var, info, &db[i], default_bpp))
> - return 4;
> + if (!fb_try_mode(var, info, &db[i], default_bpp)) {
> + ret = 4;
> + goto out;
> + }
>
> DPRINTK("No valid mode found\n");
> - return 0;
> + ret = 0;
> +out:
> + kfree(mode_option_buf);
> + return ret;
> }
>
> /**
^ permalink raw reply
* Re: [PATCH v4 02/14] mfd: lm3533: Remove driver specific regmap wrappers
From: Andy Shevchenko @ 2026-06-09 19:02 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
In-Reply-To: <CAPVz0n2rdgw8Xr3uxVdQGwrHTNFqK4SKQDFU2FEB8LzLwPhQ_A@mail.gmail.com>
On Sat, Jun 06, 2026 at 10:22:43AM +0300, Svyatoslav Ryhel wrote:
> сб, 6 черв. 2026 р. о 09:53 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
> > On Sat, Jun 06, 2026 at 07:57:26AM +0300, Svyatoslav Ryhel wrote:
...
> > > + ret = regmap_assign_bits(als->lm3533->regmap, LM3533_REG_ALS_ZONE_INFO,
> > > + LM3533_ALS_INT_ENABLE_MASK, enable);
> >
> > In cases like this perhaps leaving mask would be fine and together with
>
> I prefer to remove intermediate variables it the helper allows to
> directly pass needed value.
>
> > struct regmap *map = als->lm3533->regmap;
>
> next patch drops lm3533 so there will be als->regmap which IMHO is
> more logical instead of passing entire lm3533 to child devices.
Still it's longer than map. A local variable may help with making lines
shorter.
> > this be nice one-liner:
> >
> > ret = regmap_assign_bits(map, LM3533_REG_ALS_ZONE_INFO, mask, enable);
> >
> > > if (ret) {
> > > dev_err(&indio_dev->dev, "failed to set int mode %d\n",
> > > enable);
> >
> > In many cases it won't increase LoC count.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 04/14] mfd: lm3533: Pass only regmap and light sensor presence to child devices
From: Andy Shevchenko @ 2026-06-09 19:06 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
In-Reply-To: <20260606045738.21050-5-clamor95@gmail.com>
On Sat, Jun 06, 2026 at 07:57:28AM +0300, Svyatoslav Ryhel wrote:
> Instead of passing the entire lm3533 core data structure, only pass the
> regmap and the light sensor presence flag to child devices.
...
> struct lm3533_als {
> - struct lm3533 *lm3533;
> + struct regmap *regmap;
> struct platform_device *pdev;
And this pdev is probably not needed. But I haven't checked the whole lot of
the patches yet.
> unsigned long flags;
...
> struct lm3533_ctrlbank {
> - struct lm3533 *lm3533;
> + struct regmap *regmap;
> struct device *dev;
Ditto.
> int id;
> };
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 05/14] iio: light: lm3533-als: Remove redundant pdata helpers
From: Andy Shevchenko @ 2026-06-09 19:10 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
In-Reply-To: <20260606045738.21050-6-clamor95@gmail.com>
On Sat, Jun 06, 2026 at 07:57:29AM +0300, Svyatoslav Ryhel wrote:
> The lm3533_als_set_input_mode and lm3533_als_set_resistor functions are
> used only in lm3533_als_setup. Incorporate their code into
> lm3533_als_setup directly to simplify driver readability.
Use func() when referring to a function in the commit message.
...
> static int lm3533_als_setup(struct lm3533_als *als,
> const struct lm3533_als_platform_data *pdata)
> {
> + struct device *dev = &als->pdev->dev;
> int ret;
>
> - ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
> + ret = regmap_assign_bits(als->regmap, LM3533_REG_ALS_CONF,
> + LM3533_ALS_INPUT_MODE_MASK, pdata->pwm_mode);
> if (ret)
> - return ret;
> + return dev_err_probe(dev, ret, "failed to set input mode %d\n",
> + pdata->pwm_mode);
>
> /* ALS input is always high impedance in PWM-mode. */
> if (!pdata->pwm_mode) {
> - ret = lm3533_als_set_resistor(als, pdata->r_select);
> + if (pdata->r_select < LM3533_ALS_RESISTOR_MIN ||
> + pdata->r_select > LM3533_ALS_RESISTOR_MAX)
> + return dev_err_probe(dev, -EINVAL,
> + "invalid resistor value\n");
> +
> + ret = regmap_write(als->regmap, LM3533_REG_ALS_RESISTOR_SELECT,
> + pdata->r_select);
> if (ret)
> - return ret;
> + return dev_err_probe(dev, ret, "failed to set resistor\n");
> }
>
> return 0;
Wondering if it would be better to
/* Bail out when in PWM-mode */
if (pdata->pwm_mode)
return 0;
/* ALS input is always high impedance in PWM-mode. */
...
as the above changes almost every line in that conditional.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
page: | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox