[PATCH v3 36/36] mmc: meson-mx-sdio: Use devm_mmc_alloc_host() helper

[Martin Blumenstingl]

Hi Binbin,

On Wed, Jun 4, 2025 at 5:02 AM Binbin Zhou <zhoubb.aaron at gmail.com> wrote:
[...]
> > > -       mmc = mmc_alloc_host(sizeof(*host), &slot_pdev->dev);
> > > +       mmc = devm_mmc_alloc_host(&slot_pdev->dev, sizeof(*host));
> > This change is fine at runtime (on my Odroid-C1 board) but it can lead
> > to a use-after-free issue.
> > meson_mx_mmc_register_clks() devm_ registers two clocks and uses
> > host->controller_dev as device.
> > This leads to the fact that during driver removal struct
> > meson_mx_mmc_host is free'd before host->controller_dev, which means
> > the clocks are also free'd after struct meson_mx_mmc_host is already
> > gone. The problem here: the clocks need the struct clk_divider and
> > struct clk_fixed_factor from struct meson_mx_mmc_host.
>
> Sorry, I have a slightly different opinion, which may not necessarily
> be correct.
>
> meson_mx_mmc_host is kzalloc as `extra` in mmc_alloc_host()[1], but in
> fact, the entire host is released in mmc_host_classdev_release()[2],
> so I don't think it will affect the use of host->controller_dev.
It's well hidden, so it's tricky to spot: we have two "struct device"
in the meson-mx-sdio driver:
1. the "controller_dev" which is belongs to the struct platform_device
of the driver/device-tree node [0]
2. a second struct platform_device and thus a second struct device
whose of_node belongs to the "slot" (which is a child node of the
controller) [1]

Clock controller initialization is the same for all slots, so I chose
to link it with the controller_device.
We want to parse the mmc slot properties (bus-width, cd-gpios, cap-*,
...) from the slot node in device-tree, so we create a second
platform_device for the slot (child) node. This design was chosen
because the controller supports multiple (up to 3) slots while the mmc
core in Linux doesn't. However, it was important for me to at least be
prepared in terms of device-tree bindings in case Linux would ever
support controllers with multiple slots.

So the lifecycle during probe is:
a) controller_device is instantiated by device-tree node with
compatible = "amlogic,meson8-sdio"
b) slot device is instantiated using the "mmc-slot" compatible and
using controller_device as parent device
c) devm_mmc_alloc_host() is called
d) clock providers are devm_* registered and attached to controller_device
e) the meson-mx-sdio driver acts as clock consumer for the clock it's providing

However, if we want to remove the driver the order is:
- remove the device attached to the "mmc-slot" compatible - before
that happens struct mmc is freed (as it's parent is being removed)
which also frees struct meson_mx_mmc_host
- (struct clk_divider and struct clk_fixed_factor are linked with the
clocks which are still registered <- here we have a problem because
those two structs are part of struct meson_mx_mmc_host which has
already been freed)
- controller_device is freed - before that happens the provided clocks
are freed (as their parent is being removed)

I take your questions as feedback on how to make the code easier to
read/understand.
This is very welcome, because it's been 8 years since I introduced the
driver and I also have to think twice about some of its details.

Let me know if things are more clear now - or if you still have any
doubts/questions.


Best regards,
Martin


[0] https://elixir.bootlin.com/linux/v6.15-rc1/source/Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdio.yaml#L83
[1] https://elixir.bootlin.com/linux/v6.15-rc1/source/Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdio.yaml#L92