| Message ID | 20230628024517.1440644-1-yunchuan@nfschina.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Remove unnecessary (void*) conversions | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
Hi,
I think you missed one case:
if (mdio_id == XGENE_MDIO_RGMII) {
mdio_bus->read = xgene_mdio_rgmii_read;
mdio_bus->write = xgene_mdio_rgmii_write;
mdio_bus->priv = (void __force *)pdata;
This cast using __force is also not required.
On Wed, Jun 28, 2023 at 10:45:17AM +0800, wuych wrote:
> @@ -211,7 +211,7 @@ static void xgene_enet_wr_mdio_csr(void __iomem *base_addr,
> static int xgene_xfi_mdio_write(struct mii_bus *bus, int phy_id,
> int reg, u16 data)
> {
> - void __iomem *addr = (void __iomem *)bus->priv;
> + void __iomem *addr = bus->priv;
> int timeout = 100;
> u32 status, val;
>
> @@ -234,7 +234,7 @@ static int xgene_xfi_mdio_write(struct mii_bus *bus, int phy_id,
>
> static int xgene_xfi_mdio_read(struct mii_bus *bus, int phy_id, int reg)
> {
> - void __iomem *addr = (void __iomem *)bus->priv;
> + void __iomem *addr = bus->priv;
> u32 data, status, val;
> int timeout = 100;
These probably cause Sparse to warn whether or not the cast is there.
Given that in this case, bus->priv is initialised via:
mdio_bus->priv = (void __force *)pdata->mdio_csr_addr;
I think the simple thing is to _always_ initialise mdio_bus->priv
to point at pdata, and have xgene_xfi_mdio_*() always do:
struct xgene_mdio_pdata *pdata = bus->priv;
void __iomem *addr = pdata->mdio_csr_addr;
The extra access will be dwarfed by the time taken to perform the
access.
This change should be made with a separate patch and not combined with
the patch removing the casts in xgene_mdio_rgmii_*().
Thanks.
On 2023/6/28 17:50, Russell King (Oracle) wrote: > Hi, > > I think you missed one case: > > if (mdio_id == XGENE_MDIO_RGMII) { > mdio_bus->read = xgene_mdio_rgmii_read; > mdio_bus->write = xgene_mdio_rgmii_write; > mdio_bus->priv = (void __force *)pdata; > > This cast using __force is also not required. > > On Wed, Jun 28, 2023 at 10:45:17AM +0800, wuych wrote: >> @@ -211,7 +211,7 @@ static void xgene_enet_wr_mdio_csr(void __iomem *base_addr, >> static int xgene_xfi_mdio_write(struct mii_bus *bus, int phy_id, >> int reg, u16 data) >> { >> - void __iomem *addr = (void __iomem *)bus->priv; >> + void __iomem *addr = bus->priv; >> int timeout = 100; >> u32 status, val; >> >> @@ -234,7 +234,7 @@ static int xgene_xfi_mdio_write(struct mii_bus *bus, int phy_id, >> >> static int xgene_xfi_mdio_read(struct mii_bus *bus, int phy_id, int reg) >> { >> - void __iomem *addr = (void __iomem *)bus->priv; >> + void __iomem *addr = bus->priv; >> u32 data, status, val; >> int timeout = 100; > These probably cause Sparse to warn whether or not the cast is there. Hi, Russell King, I didn't notice this Sparse warning. Should I remove this cast although it cause Sparse warning? > > Given that in this case, bus->priv is initialised via: > > mdio_bus->priv = (void __force *)pdata->mdio_csr_addr; > > I think the simple thing is to _always_ initialise mdio_bus->priv > to point at pdata, and have xgene_xfi_mdio_*() always do: > > struct xgene_mdio_pdata *pdata = bus->priv; > void __iomem *addr = pdata->mdio_csr_addr; > > The extra access will be dwarfed by the time taken to perform the > access. > > This change should be made with a separate patch and not combined with > the patch removing the casts in xgene_mdio_rgmii_*(). yeah, this change is great. I will send a separate patch as your suggestion If we can ignore Sparse warning. Thanks for your suggestion! wuych > > Thanks. >
On Thu, Jun 29, 2023 at 09:59:56AM +0800, yunchuan wrote: > On 2023/6/28 17:50, Russell King (Oracle) wrote: > > On Wed, Jun 28, 2023 at 10:45:17AM +0800, wuych wrote: > > > @@ -211,7 +211,7 @@ static void xgene_enet_wr_mdio_csr(void __iomem *base_addr, > > > static int xgene_xfi_mdio_write(struct mii_bus *bus, int phy_id, > > > int reg, u16 data) > > > { > > > - void __iomem *addr = (void __iomem *)bus->priv; > > > + void __iomem *addr = bus->priv; > > > int timeout = 100; > > > u32 status, val; > > > @@ -234,7 +234,7 @@ static int xgene_xfi_mdio_write(struct mii_bus *bus, int phy_id, > > > static int xgene_xfi_mdio_read(struct mii_bus *bus, int phy_id, int reg) > > > { > > > - void __iomem *addr = (void __iomem *)bus->priv; > > > + void __iomem *addr = bus->priv; > > > u32 data, status, val; > > > int timeout = 100; > > These probably cause Sparse to warn whether or not the cast is there. > > Hi, Russell King, > > I didn't notice this Sparse warning. > Should I remove this cast although it cause Sparse warning? No. Don't introduce new Sparse warnings. regards, dan carpenter
On 2023/6/29 13:50, Dan Carpenter wrote: > On Thu, Jun 29, 2023 at 09:59:56AM +0800, yunchuan wrote: >> On 2023/6/28 17:50, Russell King (Oracle) wrote: >>> On Wed, Jun 28, 2023 at 10:45:17AM +0800, wuych wrote: >>>> @@ -211,7 +211,7 @@ static void xgene_enet_wr_mdio_csr(void __iomem *base_addr, >>>> static int xgene_xfi_mdio_write(struct mii_bus *bus, int phy_id, >>>> int reg, u16 data) >>>> { >>>> - void __iomem *addr = (void __iomem *)bus->priv; >>>> + void __iomem *addr = bus->priv; >>>> int timeout = 100; >>>> u32 status, val; >>>> @@ -234,7 +234,7 @@ static int xgene_xfi_mdio_write(struct mii_bus *bus, int phy_id, >>>> static int xgene_xfi_mdio_read(struct mii_bus *bus, int phy_id, int reg) >>>> { >>>> - void __iomem *addr = (void __iomem *)bus->priv; >>>> + void __iomem *addr = bus->priv; >>>> u32 data, status, val; >>>> int timeout = 100; >>> These probably cause Sparse to warn whether or not the cast is there. >> Hi, Russell King, >> >> I didn't notice this Sparse warning. >> Should I remove this cast although it cause Sparse warning? > No. Don't introduce new Sparse warnings. Got it, thanks for your answer! wuych > > regards, > dan carpenter >
diff --git a/drivers/net/mdio/mdio-xgene.c b/drivers/net/mdio/mdio-xgene.c index 7aafc221b5cf..aa79464c9d6d 100644 --- a/drivers/net/mdio/mdio-xgene.c +++ b/drivers/net/mdio/mdio-xgene.c @@ -79,7 +79,7 @@ EXPORT_SYMBOL(xgene_mdio_wr_mac); int xgene_mdio_rgmii_read(struct mii_bus *bus, int phy_id, int reg) { - struct xgene_mdio_pdata *pdata = (struct xgene_mdio_pdata *)bus->priv; + struct xgene_mdio_pdata *pdata = bus->priv; u32 data, done; u8 wait = 10; @@ -105,7 +105,7 @@ EXPORT_SYMBOL(xgene_mdio_rgmii_read); int xgene_mdio_rgmii_write(struct mii_bus *bus, int phy_id, int reg, u16 data) { - struct xgene_mdio_pdata *pdata = (struct xgene_mdio_pdata *)bus->priv; + struct xgene_mdio_pdata *pdata = bus->priv; u32 val, done; u8 wait = 10; @@ -211,7 +211,7 @@ static void xgene_enet_wr_mdio_csr(void __iomem *base_addr, static int xgene_xfi_mdio_write(struct mii_bus *bus, int phy_id, int reg, u16 data) { - void __iomem *addr = (void __iomem *)bus->priv; + void __iomem *addr = bus->priv; int timeout = 100; u32 status, val; @@ -234,7 +234,7 @@ static int xgene_xfi_mdio_write(struct mii_bus *bus, int phy_id, static int xgene_xfi_mdio_read(struct mii_bus *bus, int phy_id, int reg) { - void __iomem *addr = (void __iomem *)bus->priv; + void __iomem *addr = bus->priv; u32 data, status, val; int timeout = 100;
Pointer variables of void * type do not require type cast. Signed-off-by: wuych <yunchuan@nfschina.com> --- drivers/net/mdio/mdio-xgene.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)