| Message ID | 1550484960-2392-1-git-send-email-krzk@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | clk/driver: platform: Fix kfree() of const memory on setting driver_override | expand |
Hi Krzysztof, On Mon, Feb 18, 2019 at 11:27 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > The problem > =========== > Several device types (platform, amba, spi etc.) provide a driver_override > field. On sysfs store or during device removal, they kfree() the > existing value. > > However the users are unaware of this and set the driver_override like: > > pdev->driver_override = "exynos5-subcmu"; > > which obviously leads to error. IMHO driver_override is not meant to be set by a driver, only from userspace, for binding the device to vfio (is there another use case?). > clk: samsung: exynos5: Fix kfree() of const memory on setting > driver_override > slimbus: ngd: Fix kfree() of const memory on setting driver_override I see all users set override immediately after allocating a platform device. Can't they just allocate a platform device using the override name instead? What am I missing? Thanks! Gr{oetje,eeting}s, Geert
On Mon, 18 Feb 2019 at 11:40, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Krzysztof, > > On Mon, Feb 18, 2019 at 11:27 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > The problem > > =========== > > Several device types (platform, amba, spi etc.) provide a driver_override > > field. On sysfs store or during device removal, they kfree() the > > existing value. > > > > However the users are unaware of this and set the driver_override like: > > > > pdev->driver_override = "exynos5-subcmu"; > > > > which obviously leads to error. > > IMHO driver_override is not meant to be set by a driver, only from userspace, > for binding the device to vfio (is there another use case?). > > > clk: samsung: exynos5: Fix kfree() of const memory on setting > > driver_override > > slimbus: ngd: Fix kfree() of const memory on setting driver_override > > I see all users set override immediately after allocating a platform device. > Can't they just allocate a platform device using the override name instead? > What am I missing? For the clk-exynos5-subcmu.c case, driver registers several sub-devices. Each sub-devices is a clock controller associated with power domain. I guess the author wanted to have meaningful names of these sub-devices (DISP, CAM etc. like names of power domains), instead of just exynos5-subcmu.1, exynos5-subcmu.2 ... If driver_override should not be used for such case, then I could replace it with what you said. For the other uses of driver_override (drivers/rpmsg/rpmsg_internal.h and drivers/slimbus/qcom-ngd-ctrl.c) I don't know. Best regards, Krzysztof
Quoting Krzysztof Kozlowski (2019-02-18 03:14:29) > On Mon, 18 Feb 2019 at 11:40, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > Hi Krzysztof, > > > > On Mon, Feb 18, 2019 at 11:27 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > The problem > > > =========== > > > Several device types (platform, amba, spi etc.) provide a driver_override > > > field. On sysfs store or during device removal, they kfree() the > > > existing value. > > > > > > However the users are unaware of this and set the driver_override like: > > > > > > pdev->driver_override = "exynos5-subcmu"; > > > > > > which obviously leads to error. > > > > IMHO driver_override is not meant to be set by a driver, only from userspace, > > for binding the device to vfio (is there another use case?). > > > > > clk: samsung: exynos5: Fix kfree() of const memory on setting > > > driver_override > > > slimbus: ngd: Fix kfree() of const memory on setting driver_override > > > > I see all users set override immediately after allocating a platform device. > > Can't they just allocate a platform device using the override name instead? > > What am I missing? > > For the clk-exynos5-subcmu.c case, driver registers several > sub-devices. Each sub-devices is a clock controller associated with > power domain. I guess the author wanted to have meaningful names of > these sub-devices (DISP, CAM etc. like names of power domains), > instead of just exynos5-subcmu.1, exynos5-subcmu.2 ... > > If driver_override should not be used for such case, then I could > replace it with what you said. Looking at the introduction of the code into the platform bus makes it sound like it was all for vfio devices. If the clk driver doesn't need it for that purpose and can get by with more generic names then it seems best to avoid using it entirely. So can you do that and resend the first patch in this series too? Effectively splitting the clk parts from the larger issue of kfree()ing of const memory.
On Wed, 20 Feb 2019 at 23:01, Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Krzysztof Kozlowski (2019-02-18 03:14:29) > > On Mon, 18 Feb 2019 at 11:40, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > > Hi Krzysztof, > > > > > > On Mon, Feb 18, 2019 at 11:27 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > > The problem > > > > =========== > > > > Several device types (platform, amba, spi etc.) provide a driver_override > > > > field. On sysfs store or during device removal, they kfree() the > > > > existing value. > > > > > > > > However the users are unaware of this and set the driver_override like: > > > > > > > > pdev->driver_override = "exynos5-subcmu"; > > > > > > > > which obviously leads to error. > > > > > > IMHO driver_override is not meant to be set by a driver, only from userspace, > > > for binding the device to vfio (is there another use case?). > > > > > > > clk: samsung: exynos5: Fix kfree() of const memory on setting > > > > driver_override > > > > slimbus: ngd: Fix kfree() of const memory on setting driver_override > > > > > > I see all users set override immediately after allocating a platform device. > > > Can't they just allocate a platform device using the override name instead? > > > What am I missing? > > > > For the clk-exynos5-subcmu.c case, driver registers several > > sub-devices. Each sub-devices is a clock controller associated with > > power domain. I guess the author wanted to have meaningful names of > > these sub-devices (DISP, CAM etc. like names of power domains), > > instead of just exynos5-subcmu.1, exynos5-subcmu.2 ... > > > > If driver_override should not be used for such case, then I could > > replace it with what you said. > > Looking at the introduction of the code into the platform bus makes it > sound like it was all for vfio devices. If the clk driver doesn't need > it for that purpose and can get by with more generic names then it seems > best to avoid using it entirely. So can you do that and resend the first > patch in this series too? Effectively splitting the clk parts from the > larger issue of kfree()ing of const memory. Sure, let me send just the clk parts. BR, Krzysztof
Hi, The problem =========== Several device types (platform, amba, spi etc.) provide a driver_override field. On sysfs store or during device removal, they kfree() the existing value. However the users are unaware of this and set the driver_override like: pdev->driver_override = "exynos5-subcmu"; which obviously leads to error. Solution ======== I provided simple helper for platform device. If this approach is acceptable, I can convert also other buses, like AMBA, SPI. Dependencies and pick up order ============================== Patch 1: please pick it as is through clock tree Patch 3: Depends on patch 1 (merge conflict) and 2. Patch 4: Depends on patch 2. Best regards, Krzysztof Krzysztof Kozlowski (4): clk: samsung: exynos5: Fix possible NULL pointer exception on platform_device_alloc() failure driver: platform: Provide helper for safer setting of driver_override clk: samsung: exynos5: Fix kfree() of const memory on setting driver_override slimbus: ngd: Fix kfree() of const memory on setting driver_override drivers/base/platform.c | 63 ++++++++++++++++++++++---------- drivers/clk/samsung/clk-exynos5-subcmu.c | 12 ++++-- drivers/slimbus/qcom-ngd-ctrl.c | 2 +- include/linux/platform_device.h | 9 ++++- 4 files changed, 61 insertions(+), 25 deletions(-)