| Message ID | 20210928140721.8805-6-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add SPI Multi I/O Bus Controller support for RZ/G2L | expand |
On Tue, Sep 28, 2021 at 03:07:20PM +0100, Lad Prabhakar wrote: > RPCIF_DIRMAP_SIZE may differ on various SoC's. Instead of using > RPCIF_DIRMAP_SIZE macro use resource size to get dirmap size > which is already part of struct rpcif. > > Also make sure we return error in case devm_ioremap_resource() > fails for dirmap. > > Fixes: ca7d8b980b67 ("memory: add Renesas RPC-IF driver") > Fixes: 59e27d7c94aa ("memory: renesas-rpc-if: fix possible NULL pointer dereference of resource") > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> In general, all fine. I just think this should be split into two patches: > @@ -147,8 +147,6 @@ > #define RPCIF_PHYINT 0x0088 /* R/W */ > #define RPCIF_PHYINT_WPVAL BIT(1) > > -#define RPCIF_DIRMAP_SIZE 0x4000000 > - > static const struct regmap_range rpcif_volatile_ranges[] = { > regmap_reg_range(RPCIF_SMRDR0, RPCIF_SMRDR1), > regmap_reg_range(RPCIF_SMWDR0, RPCIF_SMWDR1), > @@ -547,8 +545,8 @@ EXPORT_SYMBOL(rpcif_manual_xfer); > > ssize_t rpcif_dirmap_read(struct rpcif *rpc, u64 offs, size_t len, void *buf) > { > - loff_t from = offs & (RPCIF_DIRMAP_SIZE - 1); > - size_t size = RPCIF_DIRMAP_SIZE - from; > + loff_t from = offs & (rpc->size - 1); > + size_t size = rpc->size - from; > > if (len > size) > len = size; This is the second patch to split which fixes ca7d8b980b67. > @@ -244,7 +242,7 @@ int rpcif_sw_init(struct rpcif *rpc, struct device *dev) > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dirmap"); > rpc->dirmap = devm_ioremap_resource(&pdev->dev, res); > if (IS_ERR(rpc->dirmap)) > - rpc->dirmap = NULL; > + return PTR_ERR(rpc->dirmap); > rpc->size = resource_size(res); > > rpc->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL); This is the first patch to split which fixes 59e27d7c94aa. Makes sense? If you agree, you can add my tag already to the new patches: Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Hi Wolfram, Thank you for the review. On Thu, Sep 30, 2021 at 2:55 PM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > On Tue, Sep 28, 2021 at 03:07:20PM +0100, Lad Prabhakar wrote: > > RPCIF_DIRMAP_SIZE may differ on various SoC's. Instead of using > > RPCIF_DIRMAP_SIZE macro use resource size to get dirmap size > > which is already part of struct rpcif. > > > > Also make sure we return error in case devm_ioremap_resource() > > fails for dirmap. > > > > Fixes: ca7d8b980b67 ("memory: add Renesas RPC-IF driver") > > Fixes: 59e27d7c94aa ("memory: renesas-rpc-if: fix possible NULL pointer dereference of resource") > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > > In general, all fine. I just think this should be split into two > patches: > Sure will split this into two. > > @@ -147,8 +147,6 @@ > > #define RPCIF_PHYINT 0x0088 /* R/W */ > > #define RPCIF_PHYINT_WPVAL BIT(1) > > > > -#define RPCIF_DIRMAP_SIZE 0x4000000 > > - > > static const struct regmap_range rpcif_volatile_ranges[] = { > > regmap_reg_range(RPCIF_SMRDR0, RPCIF_SMRDR1), > > regmap_reg_range(RPCIF_SMWDR0, RPCIF_SMWDR1), > > @@ -547,8 +545,8 @@ EXPORT_SYMBOL(rpcif_manual_xfer); > > > > ssize_t rpcif_dirmap_read(struct rpcif *rpc, u64 offs, size_t len, void *buf) > > { > > - loff_t from = offs & (RPCIF_DIRMAP_SIZE - 1); > > - size_t size = RPCIF_DIRMAP_SIZE - from; > > + loff_t from = offs & (rpc->size - 1); > > + size_t size = rpc->size - from; > > > > if (len > size) > > len = size; > > This is the second patch to split which fixes ca7d8b980b67. > Will split this change into a second patch but wont add a fixes tag see below.. > > > @@ -244,7 +242,7 @@ int rpcif_sw_init(struct rpcif *rpc, struct device *dev) > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dirmap"); > > rpc->dirmap = devm_ioremap_resource(&pdev->dev, res); > > if (IS_ERR(rpc->dirmap)) > > - rpc->dirmap = NULL; > > + return PTR_ERR(rpc->dirmap); > > rpc->size = resource_size(res); > > > > rpc->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL); > > This is the first patch to split which fixes 59e27d7c94aa. > > Makes sense? > Both the fixes would apply to the first patch itself i.e. when resource_size was added in ca7d8b980b67 and later in 59e27d7c94aa resource_size was moved online below (this would cause kernel panic res is NULL). Do you agree? > If you agree, you can add my tag already to the new patches: > > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > Sure will add your RB tag. Cheers, Prabhakar
> Both the fixes would apply to the first patch itself i.e. when > resource_size was added in ca7d8b980b67 and later in 59e27d7c94aa > resource_size was moved online below (this would cause kernel panic > res is NULL). Do you agree? Yes. I do wonder a little if we need the Fixes tag for ca7d8b980b67 because we fix 59e27d7c94aa which already fixes ca7d8b980b67, so there is a chain. But maybe redundancy doesn't hurt here. I don't mind.
diff --git a/drivers/memory/renesas-rpc-if.c b/drivers/memory/renesas-rpc-if.c index 77a011d5ff8c..140cb4e2b789 100644 --- a/drivers/memory/renesas-rpc-if.c +++ b/drivers/memory/renesas-rpc-if.c @@ -147,8 +147,6 @@ #define RPCIF_PHYINT 0x0088 /* R/W */ #define RPCIF_PHYINT_WPVAL BIT(1) -#define RPCIF_DIRMAP_SIZE 0x4000000 - static const struct regmap_range rpcif_volatile_ranges[] = { regmap_reg_range(RPCIF_SMRDR0, RPCIF_SMRDR1), regmap_reg_range(RPCIF_SMWDR0, RPCIF_SMWDR1), @@ -244,7 +242,7 @@ int rpcif_sw_init(struct rpcif *rpc, struct device *dev) res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dirmap"); rpc->dirmap = devm_ioremap_resource(&pdev->dev, res); if (IS_ERR(rpc->dirmap)) - rpc->dirmap = NULL; + return PTR_ERR(rpc->dirmap); rpc->size = resource_size(res); rpc->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL); @@ -547,8 +545,8 @@ EXPORT_SYMBOL(rpcif_manual_xfer); ssize_t rpcif_dirmap_read(struct rpcif *rpc, u64 offs, size_t len, void *buf) { - loff_t from = offs & (RPCIF_DIRMAP_SIZE - 1); - size_t size = RPCIF_DIRMAP_SIZE - from; + loff_t from = offs & (rpc->size - 1); + size_t size = rpc->size - from; if (len > size) len = size;
RPCIF_DIRMAP_SIZE may differ on various SoC's. Instead of using RPCIF_DIRMAP_SIZE macro use resource size to get dirmap size which is already part of struct rpcif. Also make sure we return error in case devm_ioremap_resource() fails for dirmap. Fixes: ca7d8b980b67 ("memory: add Renesas RPC-IF driver") Fixes: 59e27d7c94aa ("memory: renesas-rpc-if: fix possible NULL pointer dereference of resource") Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> --- drivers/memory/renesas-rpc-if.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)