[PATCH 0/3] soc: mediatek: mtk-socinfo: Fixes and cleanup

[PATCH 0/3] soc: mediatek: mtk-socinfo: Fixes and cleanup

[Chen-Yu Tsai]

Hi,

The new mtk-socinfo driver has a double put of the nvmem device used to
read the socinfo data. While fixing it, I rewrote the read function to
make better use of the device node and device relationship.

Patch 1 rewrites the cell read function in the mtk-socinfo so that no
resource leaks happen, and device lookup is more efficient.

Sidenote: I think the cell read function could be reworked a bit more
to return different error codes for different failure modes.

Patch 2 adds an extra socinfo entry for MT8183. It seems that some units
have chips that have this one. At least mine does.

Patch 3 drops the custom nvmem device name from the mtk-efuse driver.
This was previously used for nvmem device lookup, but on MT8183 with
two efuses, one would fail to probe due to this. Since after patch 1
this is no longer used, we can just drop it.

Please merge. On the MT8183 ChromeOS devices this currently crashes.


Thanks
ChenYu


Chen-Yu Tsai (3):
  soc: mediatek: mtk-socinfo: Clean up NVMEM cell read
  soc: mediatek: mtk-socinfo: Add extra entry for MT8183
  nvmem: mtk-efuse: Drop NVMEM device name

 drivers/nvmem/mtk-efuse.c          |  1 -
 drivers/soc/mediatek/mtk-socinfo.c | 17 +++++++++++------
 2 files changed, 11 insertions(+), 7 deletions(-)

-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH 1/3] soc: mediatek: mtk-socinfo: Clean up NVMEM cell read

[Chen-Yu Tsai]

The mtk-socinfo grabs the NVMEM device devm_nvmem_device_get(), but then
proceeds to put the device directly with nvmem_device_put() if the read
is successful. If the device fails to probe and goes through the devres
release path, the device would be put a second time, triggering a
use-after-free error from KASAN.

Fix this by dropping the devres part. Since the NVMEM cell data is read
only once, there is no need to keep the reference around.

While at it, clean up the function to directly reference the NVMEM
device node and use that to find the NVMEM device, instead of finding it
by name, which is more fragile. The cell node is always a direct child
of the NVMEM device node, courtesy of the legacy NVMEM cell layout. Thus
of_get_child_by_name() is a better way of finding the cell. Last,
correctly put the device node once its use is over.

Fixes: 423a54da3c7e ("soc: mediatek: mtk-socinfo: Add driver for getting chip information")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/soc/mediatek/mtk-socinfo.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-socinfo.c b/drivers/soc/mediatek/mtk-socinfo.c
index 0094f43e1e08..3909d22062ce 100644
--- a/drivers/soc/mediatek/mtk-socinfo.c
+++ b/drivers/soc/mediatek/mtk-socinfo.c
@@ -9,6 +9,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/nvmem-consumer.h>
 #include <linux/device.h>
+#include <linux/device/bus.h>
 #include <linux/debugfs.h>
 #include <linux/seq_file.h>
 #include <linux/string.h>
@@ -82,25 +83,28 @@ static int mtk_socinfo_create_socinfo_node(struct mtk_socinfo *mtk_socinfop)
 static u32 mtk_socinfo_read_cell(struct device *dev, const char *name)
 {
 	struct nvmem_device *nvmemp;
-	struct device_node *np = dev->of_node;
+	struct device_node *np, *nvmem_node = dev->parent->of_node;
 	u32 offset;
 	u32 cell_val = CELL_NOT_USED;
 
-	nvmemp = devm_nvmem_device_get(dev, "mtk-efuse0");
+	/* should never fail since the nvmem driver registers this child */
+	nvmemp = nvmem_device_find(nvmem_node, device_match_of_node);
 	if (IS_ERR(nvmemp))
 		goto out;
 
-	np = of_find_node_by_name(NULL, name);
+	np = of_get_child_by_name(nvmem_node, name);
 	if (!np)
-		goto out;
+		goto put_device;
 
 	if (of_property_read_u32_index(np, "reg", 0, &offset))
-		goto out;
+		goto put_node;
 
 	nvmem_device_read(nvmemp, offset, sizeof(cell_val), &cell_val);
 
+put_node:
+	of_node_put(np);
+put_device:
 	nvmem_device_put(nvmemp);
-
 out:
 	return cell_val;
 }
-- 
2.43.0.429.g432eaa2c6b-goog

Re: [PATCH 1/3] soc: mediatek: mtk-socinfo: Clean up NVMEM cell read

[AngeloGioacchino Del Regno]

Il 30/01/24 10:56, Chen-Yu Tsai ha scritto:
> The mtk-socinfo grabs the NVMEM device devm_nvmem_device_get(), but then
> proceeds to put the device directly with nvmem_device_put() if the read
> is successful. If the device fails to probe and goes through the devres
> release path, the device would be put a second time, triggering a
> use-after-free error from KASAN.
> 
> Fix this by dropping the devres part. Since the NVMEM cell data is read
> only once, there is no need to keep the reference around.
> 
> While at it, clean up the function to directly reference the NVMEM
> device node and use that to find the NVMEM device, instead of finding it
> by name, which is more fragile. The cell node is always a direct child
> of the NVMEM device node, courtesy of the legacy NVMEM cell layout. Thus
> of_get_child_by_name() is a better way of finding the cell. Last,
> correctly put the device node once its use is over.
> 
> Fixes: 423a54da3c7e ("soc: mediatek: mtk-socinfo: Add driver for getting chip information")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

[PATCH 2/3] soc: mediatek: mtk-socinfo: Add extra entry for MT8183

[Chen-Yu Tsai]

The MT8183 has another socinfo match, with the second cell only
differing by one bit. Add it to the driver.

Fixes: 423a54da3c7e ("soc: mediatek: mtk-socinfo: Add driver for getting chip information")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/soc/mediatek/mtk-socinfo.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/soc/mediatek/mtk-socinfo.c b/drivers/soc/mediatek/mtk-socinfo.c
index 3909d22062ce..42572e8c1520 100644
--- a/drivers/soc/mediatek/mtk-socinfo.c
+++ b/drivers/soc/mediatek/mtk-socinfo.c
@@ -45,6 +45,7 @@ static const char *cell_names[MAX_CELLS] = {"socinfo-data1", "socinfo-data2"};
 static struct socinfo_data socinfo_data_table[] = {
 	MTK_SOCINFO_ENTRY("MT8173", "MT8173V/AC", "MT8173", 0x6CA20004, 0x10000000),
 	MTK_SOCINFO_ENTRY("MT8183", "MT8183V/AZA", "Kompanio 500", 0x00010043, 0x00000840),
+	MTK_SOCINFO_ENTRY("MT8183", "MT8183V/AZA", "Kompanio 500", 0x00010043, 0x00000940),
 	MTK_SOCINFO_ENTRY("MT8186", "MT8186GV/AZA", "Kompanio 520", 0x81861001, CELL_NOT_USED),
 	MTK_SOCINFO_ENTRY("MT8186T", "MT8186TV/AZA", "Kompanio 528", 0x81862001, CELL_NOT_USED),
 	MTK_SOCINFO_ENTRY("MT8188", "MT8188GV/AZA", "Kompanio 830", 0x81880000, 0x00000010),
-- 
2.43.0.429.g432eaa2c6b-goog

Re: [PATCH 2/3] soc: mediatek: mtk-socinfo: Add extra entry for MT8183

[AngeloGioacchino Del Regno]

Il 30/01/24 10:56, Chen-Yu Tsai ha scritto:
> The MT8183 has another socinfo match, with the second cell only
> differing by one bit. Add it to the driver.
> 
> Fixes: 423a54da3c7e ("soc: mediatek: mtk-socinfo: Add driver for getting chip information")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>   drivers/soc/mediatek/mtk-socinfo.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/soc/mediatek/mtk-socinfo.c b/drivers/soc/mediatek/mtk-socinfo.c
> index 3909d22062ce..42572e8c1520 100644
> --- a/drivers/soc/mediatek/mtk-socinfo.c
> +++ b/drivers/soc/mediatek/mtk-socinfo.c
> @@ -45,6 +45,7 @@ static const char *cell_names[MAX_CELLS] = {"socinfo-data1", "socinfo-data2"};
>   static struct socinfo_data socinfo_data_table[] = {
>   	MTK_SOCINFO_ENTRY("MT8173", "MT8173V/AC", "MT8173", 0x6CA20004, 0x10000000),
>   	MTK_SOCINFO_ENTRY("MT8183", "MT8183V/AZA", "Kompanio 500", 0x00010043, 0x00000840),
> +	MTK_SOCINFO_ENTRY("MT8183", "MT8183V/AZA", "Kompanio 500", 0x00010043, 0x00000940),

Are you sure that 0x940 is the same MT8183V/AZA?

Added William-tw to the loop; MediaTek, can you please confirm, and please say why,
there are two MT8183V/AZA with two different ChipIDs?

Thanks,
Angelo

>   	MTK_SOCINFO_ENTRY("MT8186", "MT8186GV/AZA", "Kompanio 520", 0x81861001, CELL_NOT_USED),
>   	MTK_SOCINFO_ENTRY("MT8186T", "MT8186TV/AZA", "Kompanio 528", 0x81862001, CELL_NOT_USED),
>   	MTK_SOCINFO_ENTRY("MT8188", "MT8188GV/AZA", "Kompanio 830", 0x81880000, 0x00000010),

Re: [PATCH 2/3] soc: mediatek: mtk-socinfo: Add extra entry for MT8183

[William-tw Lin (林鼎崴)]

Acked-by: William-tw Lin <william-tw.lin@mediatek.com>

On Tue, 2024-01-30 at 17:56 +0800, Chen-Yu Tsai wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  The MT8183 has another socinfo match, with the second cell only
> differing by one bit. Add it to the driver.
> 
> Fixes: 423a54da3c7e ("soc: mediatek: mtk-socinfo: Add driver for
> getting chip information")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>  drivers/soc/mediatek/mtk-socinfo.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/soc/mediatek/mtk-socinfo.c
> b/drivers/soc/mediatek/mtk-socinfo.c
> index 3909d22062ce..42572e8c1520 100644
> --- a/drivers/soc/mediatek/mtk-socinfo.c
> +++ b/drivers/soc/mediatek/mtk-socinfo.c
> @@ -45,6 +45,7 @@ static const char *cell_names[MAX_CELLS] =
> {"socinfo-data1", "socinfo-data2"};
>  static struct socinfo_data socinfo_data_table[] = {
>  	MTK_SOCINFO_ENTRY("MT8173", "MT8173V/AC", "MT8173", 0x6CA20004,
> 0x10000000),
>  	MTK_SOCINFO_ENTRY("MT8183", "MT8183V/AZA", "Kompanio 500",
> 0x00010043, 0x00000840),
> +	MTK_SOCINFO_ENTRY("MT8183", "MT8183V/AZA", "Kompanio 500",
> 0x00010043, 0x00000940),
>  	MTK_SOCINFO_ENTRY("MT8186", "MT8186GV/AZA", "Kompanio 520",
> 0x81861001, CELL_NOT_USED),
>  	MTK_SOCINFO_ENTRY("MT8186T", "MT8186TV/AZA", "Kompanio 528",
> 0x81862001, CELL_NOT_USED),
>  	MTK_SOCINFO_ENTRY("MT8188", "MT8188GV/AZA", "Kompanio 830",
> 0x81880000, 0x00000010),
> -- 
> 2.43.0.429.g432eaa2c6b-goog

[PATCH 3/3] nvmem: mtk-efuse: Drop NVMEM device name

[Chen-Yu Tsai]

The MT8183 has not one but two efuse devices. The static name and ID
causes the second efuse device to fail to probe, due to duplicate sysfs
entries.

With the rework of the mtk-socinfo driver, lookup by name is no longer
necessary. The custom name can simply be dropped.

Fixes: 4e6102d60d88 ("nvmem: mtk-efuse: Register MediaTek socinfo driver from efuse")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/nvmem/mtk-efuse.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/nvmem/mtk-efuse.c b/drivers/nvmem/mtk-efuse.c
index f5bebcecf9bd..9caf04667341 100644
--- a/drivers/nvmem/mtk-efuse.c
+++ b/drivers/nvmem/mtk-efuse.c
@@ -86,7 +86,6 @@ static int mtk_efuse_probe(struct platform_device *pdev)
 	econfig.size = resource_size(res);
 	econfig.priv = priv;
 	econfig.dev = dev;
-	econfig.name = "mtk-efuse";
 	if (pdata->uses_post_processing)
 		econfig.fixup_dt_cell_info = &mtk_efuse_fixup_dt_cell_info;
 	nvmem = devm_nvmem_register(dev, &econfig);
-- 
2.43.0.429.g432eaa2c6b-goog

Re: [PATCH 3/3] nvmem: mtk-efuse: Drop NVMEM device name

[AngeloGioacchino Del Regno]

Il 30/01/24 10:56, Chen-Yu Tsai ha scritto:
> The MT8183 has not one but two efuse devices. The static name and ID
> causes the second efuse device to fail to probe, due to duplicate sysfs
> entries.
> 
> With the rework of the mtk-socinfo driver, lookup by name is no longer
> necessary. The custom name can simply be dropped.
> 
> Fixes: 4e6102d60d88 ("nvmem: mtk-efuse: Register MediaTek socinfo driver from efuse")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH 3/3] nvmem: mtk-efuse: Drop NVMEM device name

[Nícolas F. R. A. Prado]

On Tue, Jan 30, 2024 at 05:56:53PM +0800, Chen-Yu Tsai wrote:
> The MT8183 has not one but two efuse devices. The static name and ID
> causes the second efuse device to fail to probe, due to duplicate sysfs
> entries.
> 
> With the rework of the mtk-socinfo driver, lookup by name is no longer
> necessary. The custom name can simply be dropped.
> 
> Fixes: 4e6102d60d88 ("nvmem: mtk-efuse: Register MediaTek socinfo driver from efuse")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

This does fix the efuse (and display) probe issue on MT8183. FYI I've sent a
regression report for it at
https://lore.kernel.org/all/47cdeac1-121d-4b1a-a3ac-e2affc7a2fc3@notapiano

Thanks,
Nícolas

Re: [PATCH 3/3] nvmem: mtk-efuse: Drop NVMEM device name

[Srinivas Kandagatla]

On 30/01/2024 09:56, Chen-Yu Tsai wrote:
> The MT8183 has not one but two efuse devices. The static name and ID
> causes the second efuse device to fail to probe, due to duplicate sysfs
> entries.

have you considered using NVMEM_DEVID_AUTO?

--srini
> 
> With the rework of the mtk-socinfo driver, lookup by name is no longer
> necessary. The custom name can simply be dropped.
> 
> Fixes: 4e6102d60d88 ("nvmem: mtk-efuse: Register MediaTek socinfo driver from efuse")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>   drivers/nvmem/mtk-efuse.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/nvmem/mtk-efuse.c b/drivers/nvmem/mtk-efuse.c
> index f5bebcecf9bd..9caf04667341 100644
> --- a/drivers/nvmem/mtk-efuse.c
> +++ b/drivers/nvmem/mtk-efuse.c
> @@ -86,7 +86,6 @@ static int mtk_efuse_probe(struct platform_device *pdev)
>   	econfig.size = resource_size(res);
>   	econfig.priv = priv;
>   	econfig.dev = dev;
> -	econfig.name = "mtk-efuse";
>   	if (pdata->uses_post_processing)
>   		econfig.fixup_dt_cell_info = &mtk_efuse_fixup_dt_cell_info;
>   	nvmem = devm_nvmem_register(dev, &econfig);

Re: [PATCH 3/3] nvmem: mtk-efuse: Drop NVMEM device name

[Chen-Yu Tsai]

On Tue, Feb 13, 2024 at 9:07 PM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
>
> On 30/01/2024 09:56, Chen-Yu Tsai wrote:
> > The MT8183 has not one but two efuse devices. The static name and ID
> > causes the second efuse device to fail to probe, due to duplicate sysfs
> > entries.
>
> have you considered using NVMEM_DEVID_AUTO?

Yes, that was considered and what is actually implemented downstream.

However, since the reason for using a custom name no longer exists, it
seems simpler to just revert to the previous behavior, which is to let
the nvmem core deal with naming.


ChenYu

> --srini
> >
> > With the rework of the mtk-socinfo driver, lookup by name is no longer
> > necessary. The custom name can simply be dropped.
> >
> > Fixes: 4e6102d60d88 ("nvmem: mtk-efuse: Register MediaTek socinfo driver from efuse")
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> >   drivers/nvmem/mtk-efuse.c | 1 -
> >   1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/nvmem/mtk-efuse.c b/drivers/nvmem/mtk-efuse.c
> > index f5bebcecf9bd..9caf04667341 100644
> > --- a/drivers/nvmem/mtk-efuse.c
> > +++ b/drivers/nvmem/mtk-efuse.c
> > @@ -86,7 +86,6 @@ static int mtk_efuse_probe(struct platform_device *pdev)
> >       econfig.size = resource_size(res);
> >       econfig.priv = priv;
> >       econfig.dev = dev;
> > -     econfig.name = "mtk-efuse";
> >       if (pdata->uses_post_processing)
> >               econfig.fixup_dt_cell_info = &mtk_efuse_fixup_dt_cell_info;
> >       nvmem = devm_nvmem_register(dev, &econfig);

Re: [PATCH 0/3] soc: mediatek: mtk-socinfo: Fixes and cleanup

[AngeloGioacchino Del Regno]

On Tue, 30 Jan 2024 17:56:50 +0800, Chen-Yu Tsai wrote:
 > The new mtk-socinfo driver has a double put of the nvmem device used to
 > read the socinfo data. While fixing it, I rewrote the read function to
 > make better use of the device node and device relationship.
 >
 > Patch 1 rewrites the cell read function in the mtk-socinfo so that no
 > resource leaks happen, and device lookup is more efficient.
 >
 > [...]

Applied to v6.8-next/soc, thanks!

[1/3] soc: mediatek: mtk-socinfo: Clean up NVMEM cell read
       commit: 82e5d7d793e8aef1275dae266427cf048a7459d6
[2/3] soc: mediatek: mtk-socinfo: Add extra entry for MT8183
       commit: 54d21dea6a6c117f3cab4caa1f9c3ffafb515dd6

Cheers,
Angelo

Re: (subset) [PATCH 0/3] soc: mediatek: mtk-socinfo: Fixes and cleanup

[Srinivas Kandagatla]

On Tue, 30 Jan 2024 17:56:50 +0800, Chen-Yu Tsai wrote:
> The new mtk-socinfo driver has a double put of the nvmem device used to
> read the socinfo data. While fixing it, I rewrote the read function to
> make better use of the device node and device relationship.
> 
> Patch 1 rewrites the cell read function in the mtk-socinfo so that no
> resource leaks happen, and device lookup is more efficient.
> 
> [...]

Applied, thanks!

[3/3] nvmem: mtk-efuse: Drop NVMEM device name
      commit: eadaa6f9aaf682b1840bf08a8320907de4e32d50

Best regards,
-- 
Srinivas Kandagatla <srinivas.kandagatla@linaro.org>