| Message ID | 20230221052237.1078627-1-c@jia.je |
|---|---|
| State | Changes Requested |
| Delegated to: | Michal Simek |
| Headers | show |
| Series | spi: xilinx_spi: Fix potential null pointer access | expand |
On 2/21/23 06:22, Jiajie Chen wrote: > It was incorrectly using an old priv->regs pointer, and may lead to null > pointer access. I would describe it a little bit differently to describe what it happening. priv structure is initiated by DM core to zeros that's why regs property is pointing to 0 address + spi offsets. That's why likely spi resets never happened. > > Signed-off-by: Jiajie Chen <c@jia.je> > --- > > drivers/spi/xilinx_spi.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c > index 4e9115dafe..e759b66000 100644 > --- a/drivers/spi/xilinx_spi.c > +++ b/drivers/spi/xilinx_spi.c > @@ -112,9 +112,7 @@ struct xilinx_spi_priv { > static int xilinx_spi_probe(struct udevice *bus) > { > struct xilinx_spi_priv *priv = dev_get_priv(bus); > - struct xilinx_spi_regs *regs = priv->regs; > - > - priv->regs = (struct xilinx_spi_regs *)dev_read_addr(bus); > + struct xilinx_spi_regs *regs = priv->regs = (struct xilinx_spi_regs *)dev_read_addr(bus); The patch is good but I pretty much don't like this long line. Can you please do it on 2 lines instead? It would be easier to read. Thanks, Michal
Comments below. On 2023/2/27 22:51, Michal Simek wrote: > > > On 2/21/23 06:22, Jiajie Chen wrote: >> It was incorrectly using an old priv->regs pointer, and may lead to null >> pointer access. > > I would describe it a little bit differently to describe what it > happening. > > priv structure is initiated by DM core to zeros that's why regs > property is pointing to 0 address + spi offsets. That's why likely spi > resets never happened. > Thanks, I will post a v2 patch. > >> >> Signed-off-by: Jiajie Chen <c@jia.je> >> --- >> >> drivers/spi/xilinx_spi.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c >> index 4e9115dafe..e759b66000 100644 >> --- a/drivers/spi/xilinx_spi.c >> +++ b/drivers/spi/xilinx_spi.c >> @@ -112,9 +112,7 @@ struct xilinx_spi_priv { >> static int xilinx_spi_probe(struct udevice *bus) >> { >> struct xilinx_spi_priv *priv = dev_get_priv(bus); >> - struct xilinx_spi_regs *regs = priv->regs; >> - >> - priv->regs = (struct xilinx_spi_regs *)dev_read_addr(bus); >> + struct xilinx_spi_regs *regs = priv->regs = (struct >> xilinx_spi_regs *)dev_read_addr(bus); > > > The patch is good but I pretty much don't like this long line. > Can you please do it on 2 lines instead? Sure. > > It would be easier to read. > > Thanks, > Michal
diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c index 4e9115dafe..e759b66000 100644 --- a/drivers/spi/xilinx_spi.c +++ b/drivers/spi/xilinx_spi.c @@ -112,9 +112,7 @@ struct xilinx_spi_priv { static int xilinx_spi_probe(struct udevice *bus) { struct xilinx_spi_priv *priv = dev_get_priv(bus); - struct xilinx_spi_regs *regs = priv->regs; - - priv->regs = (struct xilinx_spi_regs *)dev_read_addr(bus); + struct xilinx_spi_regs *regs = priv->regs = (struct xilinx_spi_regs *)dev_read_addr(bus); priv->fifo_depth = dev_read_u32_default(bus, "fifo-size", 0);
It was incorrectly using an old priv->regs pointer, and may lead to null pointer access. Signed-off-by: Jiajie Chen <c@jia.je> --- drivers/spi/xilinx_spi.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)