Re: [PATCH 2/2] iio: iio-mux: kzalloc instead of devm_kzalloc to ensure page alignment

[Jonathan Cameron]

On Sun, 12 Jan 2025 18:24:30 +0200
Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:

> Mon, Dec 09, 2024 at 11:39:55AM +0100, Matteo Martelli kirjoitti:
> > On Sun, 8 Dec 2024 18:15:31 +0000, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:  
> > > On Mon, 02 Dec 2024 16:11:08 +0100
> > > Matteo Martelli <matteomartelli3@xxxxxxxxx> wrote:
> > >   
> > > > During channel configuration, the iio-mux driver allocates a page with
> > > > devm_kzalloc(PAGE_SIZE) to read channel ext_info. However, the resulting
> > > > buffer points to an offset of the page due to the devres header sitting
> > > > at the beginning of the allocated area. This leads to failure in the
> > > > provider driver when sysfs_emit* helpers are used to format the ext_info
> > > > attributes.
> > > > 
> > > > Switch to plain kzalloc version. The devres version is not strictly
> > > > necessary as the buffer is only accessed during the channel
> > > > configuration phase. Rely on __free cleanup to deallocate the buffer.
> > > > Also, move the ext_info handling into a new function to have the page
> > > > buffer definition and assignment in one statement as suggested by
> > > > cleanup documentation.
> > > > 
> > > > Signed-off-by: Matteo Martelli <matteomartelli3@xxxxxxxxx>  
> > > This seems fine to me, but the diff ended up a bit complex, so I'd like
> > > Peter to take a look as well before I apply it.  
> > 
> > For a simpler diff I could go for devm_get_free_pages()+devm_free_pages(),
> > but since devres doesn't seem necessary in this case, I think this patch
> > provides a cleaner solution at the end.  
> 
> I am here just to ask why we don't use get_free_page() or analogue to begin
> with, but it seems the thread has a remark WRT this.
This patch has now gone upstream, but a follow up to switch to
get_free_page() would be fine as a cleanup.

I just wanted to land the fix reasonably quickly rather than waiting
for the perfect answer.

Jonathan

> 
> > > Do you have a board that is hitting this?  If so, a fixes tag is definitely
> > > appropriate. I think it is probably appropriate even it not.  
> > 
> > I am not sure if any existing board is affected as I encountered this
> > issue while experimenting with consumer drivers, thus using a custom DT
> > on top of sun50i-a64-pine64.dts just for testing. The following DT files
> > might be affected but only if the iio channel controlled by the iio_mux
> > multiplexer owns an ext_info attribute which is also exposed on sysfs.
> > 
> > $ grep -Rl 'io-channel-mux' arch  
> 
> Btw, `git grep ...` is _much_ faster.
> 
> 	git grep -lw io-channel-mux -- arch/
> 
> and won't give unrelated lines (e.g., *.dtb).
> 
> > arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtmitchell.dts
> > arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtjade.dts
> > arch/arm/boot/dts/microchip/at91-tse850-3.dts
> > arch/arm/boot/dts/microchip/at91-natte.dtsi
> > arch/arm64/boot/dts/rockchip/rk3566-powkiddy-rk2023.dtsi
> > arch/arm64/boot/dts/rockchip/rk3566-anbernic-rg353x.dtsi
> > arch/arm64/boot/dts/rockchip/rk3566-anbernic-rg503.dts
> > arch/arm64/boot/dts/rockchip/rk3326-odroid-go3.dts
> > arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dtb
> > arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts  
>