| Message ID | 20220124122132.435743-1-christian.gmeiner@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | Revert "PCI: j721e: Drop redundant struct device *" | expand |
On Mon, Jan 24, 2022 at 6:21 AM Christian Gmeiner <christian.gmeiner@gmail.com> wrote: > > This reverts commit 19e863828acf6d8ac8475ba1fd93c0fe17fdc4ef. > > Fixes the following oops: Perhaps explain why the 2nd struct device was not redundant. Is this not just a case of the dev pointer not getting set early enough? > Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010 > Internal error: Oops: 96000004 [#1] PREEMPT SMP > Modules linked in: > CPU: 1 PID: 7 Comm: kworker/u4:0 Not tainted 5.17.0-rc1-00086-ge38b27816fea-dirty #71 > Hardware name: CPE0108 (DT) > Workqueue: events_unbound deferred_probe_work_func > pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > pc : j721e_pcie_probe+0x184/0x600 > lr : j721e_pcie_probe+0x170/0x600 > sp : ffff80000957bae0 > x29: ffff80000957bae0 x28: ffff800009357000 x27: ffff00000000c078 > x26: ffff00003fe047a8 x25: 0000000000000000 x24: ffff0000000f5280 > x23: ffff800008c98f78 x22: ffff800008f90ff0 x21: ffff000000231410 > x20: ffff000002ef2780 x19: 0000000000000021 x18: 0000000000000001 > x17: 0000000000000000 x16: 0000000000058c00 x15: ffffffffffffffff > x14: ffffffffffffffff x13: 0000000000000010 x12: 0101010101010101 > x11: 0000000000000040 x10: ffff8000093e06c8 x9 : ffff8000093e06c0 > x8 : ffff000000400270 x7 : 0000000000000000 x6 : ffff000000231590 > x5 : ffff80000957b9e0 x4 : 0000000000000000 x3 : ffff0000002314f4 > x2 : 0000000000000000 x1 : ffff0000000f5280 x0 : 0000000000000000 > Call trace: > j721e_pcie_probe+0x184/0x600 > platform_probe+0x68/0xe0 > really_probe+0x144/0x320 > __driver_probe_device+0xc4/0xe0 > driver_probe_device+0x7c/0x110 > __device_attach_driver+0x90/0xe0 > bus_for_each_drv+0x78/0xd0 > __device_attach+0xf0/0x150 > device_initial_probe+0x14/0x20 > bus_probe_device+0x9c/0xb0 > deferred_probe_work_func+0x88/0xc0 > process_one_work+0x1bc/0x340 > worker_thread+0x1f8/0x420 > kthread+0x110/0x120 > ret_from_fork+0x10/0x20 > Code: f9400280 a90573fb d0005396 913fc2d6 (f9400800) > > Fixes: 19e863828acf ("PCI: j721e: Drop redundant struct device *") > Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com> > --- > drivers/pci/controller/cadence/pci-j721e.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c > index 489586a4cdc7..cd43d1898482 100644 > --- a/drivers/pci/controller/cadence/pci-j721e.c > +++ b/drivers/pci/controller/cadence/pci-j721e.c > @@ -51,10 +51,11 @@ enum link_status { > #define MAX_LANES 2 > > struct j721e_pcie { > - struct cdns_pcie *cdns_pcie; > + struct device *dev; > struct clk *refclk; > u32 mode; > u32 num_lanes; > + struct cdns_pcie *cdns_pcie; > void __iomem *user_cfg_base; > void __iomem *intd_cfg_base; > u32 linkdown_irq_regfield; > @@ -98,7 +99,7 @@ static inline void j721e_pcie_intd_writel(struct j721e_pcie *pcie, u32 offset, > static irqreturn_t j721e_pcie_link_irq_handler(int irq, void *priv) > { > struct j721e_pcie *pcie = priv; > - struct device *dev = pcie->cdns_pcie->dev; > + struct device *dev = pcie->dev; > u32 reg; > > reg = j721e_pcie_intd_readl(pcie, STATUS_REG_SYS_2); > @@ -164,7 +165,7 @@ static const struct cdns_pcie_ops j721e_pcie_ops = { > static int j721e_pcie_set_mode(struct j721e_pcie *pcie, struct regmap *syscon, > unsigned int offset) > { > - struct device *dev = pcie->cdns_pcie->dev; > + struct device *dev = pcie->dev; > u32 mask = J721E_MODE_RC; > u32 mode = pcie->mode; > u32 val = 0; > @@ -183,7 +184,7 @@ static int j721e_pcie_set_mode(struct j721e_pcie *pcie, struct regmap *syscon, > static int j721e_pcie_set_link_speed(struct j721e_pcie *pcie, > struct regmap *syscon, unsigned int offset) > { > - struct device *dev = pcie->cdns_pcie->dev; > + struct device *dev = pcie->dev; > struct device_node *np = dev->of_node; > int link_speed; > u32 val = 0; > @@ -204,7 +205,7 @@ static int j721e_pcie_set_link_speed(struct j721e_pcie *pcie, > static int j721e_pcie_set_lane_count(struct j721e_pcie *pcie, > struct regmap *syscon, unsigned int offset) > { > - struct device *dev = pcie->cdns_pcie->dev; > + struct device *dev = pcie->dev; > u32 lanes = pcie->num_lanes; > u32 val = 0; > int ret; > @@ -219,7 +220,7 @@ static int j721e_pcie_set_lane_count(struct j721e_pcie *pcie, > > static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie) > { > - struct device *dev = pcie->cdns_pcie->dev; > + struct device *dev = pcie->dev; > struct device_node *node = dev->of_node; > struct of_phandle_args args; > unsigned int offset = 0; > @@ -376,6 +377,7 @@ static int j721e_pcie_probe(struct platform_device *pdev) > if (!pcie) > return -ENOMEM; > > + pcie->dev = dev; > pcie->mode = mode; > pcie->linkdown_irq_regfield = data->linkdown_irq_regfield; > > -- > 2.34.1 >
On Mon, Jan 24, 2022 at 01:21:22PM +0100, Christian Gmeiner wrote: > This reverts commit 19e863828acf6d8ac8475ba1fd93c0fe17fdc4ef. > > Fixes the following oops: > Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010 My fault, sorry for breaking this. Thanks a lot for the report! 19e863828acf ("PCI: j721e: Drop redundant struct device *") failed to consider the uses of pcie->cdns_pcie->dev before pcie->cdns_pcie is initialized. I'll figure out what to do about it. > Internal error: Oops: 96000004 [#1] PREEMPT SMP > Modules linked in: > CPU: 1 PID: 7 Comm: kworker/u4:0 Not tainted 5.17.0-rc1-00086-ge38b27816fea-dirty #71 > Hardware name: CPE0108 (DT) > Workqueue: events_unbound deferred_probe_work_func > pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > pc : j721e_pcie_probe+0x184/0x600 > lr : j721e_pcie_probe+0x170/0x600 > sp : ffff80000957bae0 > x29: ffff80000957bae0 x28: ffff800009357000 x27: ffff00000000c078 > x26: ffff00003fe047a8 x25: 0000000000000000 x24: ffff0000000f5280 > x23: ffff800008c98f78 x22: ffff800008f90ff0 x21: ffff000000231410 > x20: ffff000002ef2780 x19: 0000000000000021 x18: 0000000000000001 > x17: 0000000000000000 x16: 0000000000058c00 x15: ffffffffffffffff > x14: ffffffffffffffff x13: 0000000000000010 x12: 0101010101010101 > x11: 0000000000000040 x10: ffff8000093e06c8 x9 : ffff8000093e06c0 > x8 : ffff000000400270 x7 : 0000000000000000 x6 : ffff000000231590 > x5 : ffff80000957b9e0 x4 : 0000000000000000 x3 : ffff0000002314f4 > x2 : 0000000000000000 x1 : ffff0000000f5280 x0 : 0000000000000000 > Call trace: > j721e_pcie_probe+0x184/0x600 > platform_probe+0x68/0xe0 > really_probe+0x144/0x320 > __driver_probe_device+0xc4/0xe0 > driver_probe_device+0x7c/0x110 > __device_attach_driver+0x90/0xe0 > bus_for_each_drv+0x78/0xd0 > __device_attach+0xf0/0x150 > device_initial_probe+0x14/0x20 > bus_probe_device+0x9c/0xb0 > deferred_probe_work_func+0x88/0xc0 > process_one_work+0x1bc/0x340 > worker_thread+0x1f8/0x420 > kthread+0x110/0x120 > ret_from_fork+0x10/0x20 > Code: f9400280 a90573fb d0005396 913fc2d6 (f9400800) > > Fixes: 19e863828acf ("PCI: j721e: Drop redundant struct device *") > Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com> > --- > drivers/pci/controller/cadence/pci-j721e.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c > index 489586a4cdc7..cd43d1898482 100644 > --- a/drivers/pci/controller/cadence/pci-j721e.c > +++ b/drivers/pci/controller/cadence/pci-j721e.c > @@ -51,10 +51,11 @@ enum link_status { > #define MAX_LANES 2 > > struct j721e_pcie { > - struct cdns_pcie *cdns_pcie; > + struct device *dev; > struct clk *refclk; > u32 mode; > u32 num_lanes; > + struct cdns_pcie *cdns_pcie; > void __iomem *user_cfg_base; > void __iomem *intd_cfg_base; > u32 linkdown_irq_regfield; > @@ -98,7 +99,7 @@ static inline void j721e_pcie_intd_writel(struct j721e_pcie *pcie, u32 offset, > static irqreturn_t j721e_pcie_link_irq_handler(int irq, void *priv) > { > struct j721e_pcie *pcie = priv; > - struct device *dev = pcie->cdns_pcie->dev; > + struct device *dev = pcie->dev; > u32 reg; > > reg = j721e_pcie_intd_readl(pcie, STATUS_REG_SYS_2); > @@ -164,7 +165,7 @@ static const struct cdns_pcie_ops j721e_pcie_ops = { > static int j721e_pcie_set_mode(struct j721e_pcie *pcie, struct regmap *syscon, > unsigned int offset) > { > - struct device *dev = pcie->cdns_pcie->dev; > + struct device *dev = pcie->dev; > u32 mask = J721E_MODE_RC; > u32 mode = pcie->mode; > u32 val = 0; > @@ -183,7 +184,7 @@ static int j721e_pcie_set_mode(struct j721e_pcie *pcie, struct regmap *syscon, > static int j721e_pcie_set_link_speed(struct j721e_pcie *pcie, > struct regmap *syscon, unsigned int offset) > { > - struct device *dev = pcie->cdns_pcie->dev; > + struct device *dev = pcie->dev; > struct device_node *np = dev->of_node; > int link_speed; > u32 val = 0; > @@ -204,7 +205,7 @@ static int j721e_pcie_set_link_speed(struct j721e_pcie *pcie, > static int j721e_pcie_set_lane_count(struct j721e_pcie *pcie, > struct regmap *syscon, unsigned int offset) > { > - struct device *dev = pcie->cdns_pcie->dev; > + struct device *dev = pcie->dev; > u32 lanes = pcie->num_lanes; > u32 val = 0; > int ret; > @@ -219,7 +220,7 @@ static int j721e_pcie_set_lane_count(struct j721e_pcie *pcie, > > static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie) > { > - struct device *dev = pcie->cdns_pcie->dev; > + struct device *dev = pcie->dev; > struct device_node *node = dev->of_node; > struct of_phandle_args args; > unsigned int offset = 0; > @@ -376,6 +377,7 @@ static int j721e_pcie_probe(struct platform_device *pdev) > if (!pcie) > return -ENOMEM; > > + pcie->dev = dev; > pcie->mode = mode; > pcie->linkdown_irq_regfield = data->linkdown_irq_regfield; > > -- > 2.34.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Jan 24, 2022 at 01:21:22PM +0100, Christian Gmeiner wrote: > This reverts commit 19e863828acf6d8ac8475ba1fd93c0fe17fdc4ef. > > Fixes the following oops: > Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010 Hi Christian, Would you mind trying this patch? commit 9d36a93af8fe ("PCI: j721e: Initialize pcie->cdns_pcie before using it") Author: Bjorn Helgaas <bhelgaas@google.com> Date: Thu Jan 27 15:49:49 2022 -0600 PCI: j721e: Initialize pcie->cdns_pcie before using it Christian reported a NULL pointer dereference in j721e_pcie_probe() caused by 19e863828acf ("PCI: j721e: Drop redundant struct device *"), which removed struct j721e_pcie.dev since there's another copy in struct cdns_pcie.dev reachable via j721e_pcie->cdns_pcie->dev. The problem is that j721e_pcie->cdns_pcie was dereferenced before being initialized: j721e_pcie_probe pcie = devm_kzalloc() # struct j721e_pcie j721e_pcie_ctrl_init(pcie) dev = pcie->cdns_pcie->dev <-- dereference cdns_pcie switch (mode) { case PCI_MODE_RC: cdns_pcie = ... # alloc as part of pci_host_bridge pcie->cdns_pcie = cdns_pcie <-- initialize pcie->cdns_pcie Initialize pcie->cdns_pcie before it is used. Fixes: 19e863828acf ("PCI: j721e: Drop redundant struct device *") Reported-by: Christian Gmeiner <christian.gmeiner@gmail.com> Link: https://lore.kernel.org/r/20220124122132.435743-1-christian.gmeiner@gmail.com Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c index 489586a4cdc7..5d950c1d9fd0 100644 --- a/drivers/pci/controller/cadence/pci-j721e.c +++ b/drivers/pci/controller/cadence/pci-j721e.c @@ -372,10 +372,48 @@ static int j721e_pcie_probe(struct platform_device *pdev) mode = (u32)data->mode; + switch (mode) { + case PCI_MODE_RC: + if (!IS_ENABLED(CONFIG_PCIE_CADENCE_HOST)) + return -ENODEV; + + bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc)); + if (!bridge) + return -ENOMEM; + + if (!data->byte_access_allowed) + bridge->ops = &cdns_ti_pcie_host_ops; + rc = pci_host_bridge_priv(bridge); + rc->quirk_retrain_flag = data->quirk_retrain_flag; + rc->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag; + + cdns_pcie = &rc->pcie; + break; + case PCI_MODE_EP: + if (!IS_ENABLED(CONFIG_PCIE_CADENCE_EP)) + return -ENODEV; + + ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL); + if (!ep) + return -ENOMEM; + + ep->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag; + + cdns_pcie = &ep->pcie; + break; + default: + dev_err(dev, "INVALID device type %d\n", mode); + return 0; + } + + cdns_pcie->dev = dev; + cdns_pcie->ops = &j721e_pcie_ops; + pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL); if (!pcie) return -ENOMEM; + pcie->cdns_pcie = cdns_pcie; pcie->mode = mode; pcie->linkdown_irq_regfield = data->linkdown_irq_regfield; @@ -426,28 +464,6 @@ static int j721e_pcie_probe(struct platform_device *pdev) switch (mode) { case PCI_MODE_RC: - if (!IS_ENABLED(CONFIG_PCIE_CADENCE_HOST)) { - ret = -ENODEV; - goto err_get_sync; - } - - bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc)); - if (!bridge) { - ret = -ENOMEM; - goto err_get_sync; - } - - if (!data->byte_access_allowed) - bridge->ops = &cdns_ti_pcie_host_ops; - rc = pci_host_bridge_priv(bridge); - rc->quirk_retrain_flag = data->quirk_retrain_flag; - rc->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag; - - cdns_pcie = &rc->pcie; - cdns_pcie->dev = dev; - cdns_pcie->ops = &j721e_pcie_ops; - pcie->cdns_pcie = cdns_pcie; - gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); if (IS_ERR(gpiod)) { ret = PTR_ERR(gpiod); @@ -497,23 +513,6 @@ static int j721e_pcie_probe(struct platform_device *pdev) break; case PCI_MODE_EP: - if (!IS_ENABLED(CONFIG_PCIE_CADENCE_EP)) { - ret = -ENODEV; - goto err_get_sync; - } - - ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL); - if (!ep) { - ret = -ENOMEM; - goto err_get_sync; - } - ep->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag; - - cdns_pcie = &ep->pcie; - cdns_pcie->dev = dev; - cdns_pcie->ops = &j721e_pcie_ops; - pcie->cdns_pcie = cdns_pcie; - ret = cdns_pcie_init_phy(dev, cdns_pcie); if (ret) { dev_err(dev, "Failed to init phy\n"); @@ -525,8 +524,6 @@ static int j721e_pcie_probe(struct platform_device *pdev) goto err_pcie_setup; break; - default: - dev_err(dev, "INVALID device type %d\n", mode); } return 0;
Am Do., 27. Jan. 2022 um 23:29 Uhr schrieb Bjorn Helgaas <helgaas@kernel.org>: > > On Mon, Jan 24, 2022 at 01:21:22PM +0100, Christian Gmeiner wrote: > > This reverts commit 19e863828acf6d8ac8475ba1fd93c0fe17fdc4ef. > > > > Fixes the following oops: > > Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010 > > Hi Christian, > > Would you mind trying this patch? > Works - thanks. You can add my Tested-by. > commit 9d36a93af8fe ("PCI: j721e: Initialize pcie->cdns_pcie before using it") > Author: Bjorn Helgaas <bhelgaas@google.com> > Date: Thu Jan 27 15:49:49 2022 -0600 > > PCI: j721e: Initialize pcie->cdns_pcie before using it > > Christian reported a NULL pointer dereference in j721e_pcie_probe() caused > by 19e863828acf ("PCI: j721e: Drop redundant struct device *"), which > removed struct j721e_pcie.dev since there's another copy in struct > cdns_pcie.dev reachable via j721e_pcie->cdns_pcie->dev. > > The problem is that j721e_pcie->cdns_pcie was dereferenced before being > initialized: > > j721e_pcie_probe > pcie = devm_kzalloc() # struct j721e_pcie > j721e_pcie_ctrl_init(pcie) > dev = pcie->cdns_pcie->dev <-- dereference cdns_pcie > switch (mode) { > case PCI_MODE_RC: > cdns_pcie = ... # alloc as part of pci_host_bridge > pcie->cdns_pcie = cdns_pcie <-- initialize pcie->cdns_pcie > > Initialize pcie->cdns_pcie before it is used. > > Fixes: 19e863828acf ("PCI: j721e: Drop redundant struct device *") > Reported-by: Christian Gmeiner <christian.gmeiner@gmail.com> > Link: https://lore.kernel.org/r/20220124122132.435743-1-christian.gmeiner@gmail.com > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c > index 489586a4cdc7..5d950c1d9fd0 100644 > --- a/drivers/pci/controller/cadence/pci-j721e.c > +++ b/drivers/pci/controller/cadence/pci-j721e.c > @@ -372,10 +372,48 @@ static int j721e_pcie_probe(struct platform_device *pdev) > > mode = (u32)data->mode; > > + switch (mode) { > + case PCI_MODE_RC: > + if (!IS_ENABLED(CONFIG_PCIE_CADENCE_HOST)) > + return -ENODEV; > + > + bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc)); > + if (!bridge) > + return -ENOMEM; > + > + if (!data->byte_access_allowed) > + bridge->ops = &cdns_ti_pcie_host_ops; > + rc = pci_host_bridge_priv(bridge); > + rc->quirk_retrain_flag = data->quirk_retrain_flag; > + rc->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag; > + > + cdns_pcie = &rc->pcie; > + break; > + case PCI_MODE_EP: > + if (!IS_ENABLED(CONFIG_PCIE_CADENCE_EP)) > + return -ENODEV; > + > + ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL); > + if (!ep) > + return -ENOMEM; > + > + ep->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag; > + > + cdns_pcie = &ep->pcie; > + break; > + default: > + dev_err(dev, "INVALID device type %d\n", mode); > + return 0; > + } > + > + cdns_pcie->dev = dev; > + cdns_pcie->ops = &j721e_pcie_ops; > + > pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL); > if (!pcie) > return -ENOMEM; > > + pcie->cdns_pcie = cdns_pcie; > pcie->mode = mode; > pcie->linkdown_irq_regfield = data->linkdown_irq_regfield; > > @@ -426,28 +464,6 @@ static int j721e_pcie_probe(struct platform_device *pdev) > > switch (mode) { > case PCI_MODE_RC: > - if (!IS_ENABLED(CONFIG_PCIE_CADENCE_HOST)) { > - ret = -ENODEV; > - goto err_get_sync; > - } > - > - bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc)); > - if (!bridge) { > - ret = -ENOMEM; > - goto err_get_sync; > - } > - > - if (!data->byte_access_allowed) > - bridge->ops = &cdns_ti_pcie_host_ops; > - rc = pci_host_bridge_priv(bridge); > - rc->quirk_retrain_flag = data->quirk_retrain_flag; > - rc->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag; > - > - cdns_pcie = &rc->pcie; > - cdns_pcie->dev = dev; > - cdns_pcie->ops = &j721e_pcie_ops; > - pcie->cdns_pcie = cdns_pcie; > - > gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); > if (IS_ERR(gpiod)) { > ret = PTR_ERR(gpiod); > @@ -497,23 +513,6 @@ static int j721e_pcie_probe(struct platform_device *pdev) > > break; > case PCI_MODE_EP: > - if (!IS_ENABLED(CONFIG_PCIE_CADENCE_EP)) { > - ret = -ENODEV; > - goto err_get_sync; > - } > - > - ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL); > - if (!ep) { > - ret = -ENOMEM; > - goto err_get_sync; > - } > - ep->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag; > - > - cdns_pcie = &ep->pcie; > - cdns_pcie->dev = dev; > - cdns_pcie->ops = &j721e_pcie_ops; > - pcie->cdns_pcie = cdns_pcie; > - > ret = cdns_pcie_init_phy(dev, cdns_pcie); > if (ret) { > dev_err(dev, "Failed to init phy\n"); > @@ -525,8 +524,6 @@ static int j721e_pcie_probe(struct platform_device *pdev) > goto err_pcie_setup; > > break; > - default: > - dev_err(dev, "INVALID device type %d\n", mode); > } > > return 0;
On Mon, Jan 31, 2022 at 12:06:21PM +0100, Christian Gmeiner wrote: > Am Do., 27. Jan. 2022 um 23:29 Uhr schrieb Bjorn Helgaas <helgaas@kernel.org>: > > > > On Mon, Jan 24, 2022 at 01:21:22PM +0100, Christian Gmeiner wrote: > > > This reverts commit 19e863828acf6d8ac8475ba1fd93c0fe17fdc4ef. > > > > > > Fixes the following oops: > > > Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010 > > > > Hi Christian, > > > > Would you mind trying this patch? > > > > Works - thanks. You can add my Tested-by. Thanks, applied to for-linus for v5.17. > > > commit 9d36a93af8fe ("PCI: j721e: Initialize pcie->cdns_pcie before using it") > > Author: Bjorn Helgaas <bhelgaas@google.com> > > Date: Thu Jan 27 15:49:49 2022 -0600 > > > > PCI: j721e: Initialize pcie->cdns_pcie before using it > > > > Christian reported a NULL pointer dereference in j721e_pcie_probe() caused > > by 19e863828acf ("PCI: j721e: Drop redundant struct device *"), which > > removed struct j721e_pcie.dev since there's another copy in struct > > cdns_pcie.dev reachable via j721e_pcie->cdns_pcie->dev. > > > > The problem is that j721e_pcie->cdns_pcie was dereferenced before being > > initialized: > > > > j721e_pcie_probe > > pcie = devm_kzalloc() # struct j721e_pcie > > j721e_pcie_ctrl_init(pcie) > > dev = pcie->cdns_pcie->dev <-- dereference cdns_pcie > > switch (mode) { > > case PCI_MODE_RC: > > cdns_pcie = ... # alloc as part of pci_host_bridge > > pcie->cdns_pcie = cdns_pcie <-- initialize pcie->cdns_pcie > > > > Initialize pcie->cdns_pcie before it is used. > > > > Fixes: 19e863828acf ("PCI: j721e: Drop redundant struct device *") > > Reported-by: Christian Gmeiner <christian.gmeiner@gmail.com> > > Link: https://lore.kernel.org/r/20220124122132.435743-1-christian.gmeiner@gmail.com > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > > > diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c > > index 489586a4cdc7..5d950c1d9fd0 100644 > > --- a/drivers/pci/controller/cadence/pci-j721e.c > > +++ b/drivers/pci/controller/cadence/pci-j721e.c > > @@ -372,10 +372,48 @@ static int j721e_pcie_probe(struct platform_device *pdev) > > > > mode = (u32)data->mode; > > > > + switch (mode) { > > + case PCI_MODE_RC: > > + if (!IS_ENABLED(CONFIG_PCIE_CADENCE_HOST)) > > + return -ENODEV; > > + > > + bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc)); > > + if (!bridge) > > + return -ENOMEM; > > + > > + if (!data->byte_access_allowed) > > + bridge->ops = &cdns_ti_pcie_host_ops; > > + rc = pci_host_bridge_priv(bridge); > > + rc->quirk_retrain_flag = data->quirk_retrain_flag; > > + rc->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag; > > + > > + cdns_pcie = &rc->pcie; > > + break; > > + case PCI_MODE_EP: > > + if (!IS_ENABLED(CONFIG_PCIE_CADENCE_EP)) > > + return -ENODEV; > > + > > + ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL); > > + if (!ep) > > + return -ENOMEM; > > + > > + ep->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag; > > + > > + cdns_pcie = &ep->pcie; > > + break; > > + default: > > + dev_err(dev, "INVALID device type %d\n", mode); > > + return 0; > > + } > > + > > + cdns_pcie->dev = dev; > > + cdns_pcie->ops = &j721e_pcie_ops; > > + > > pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL); > > if (!pcie) > > return -ENOMEM; > > > > + pcie->cdns_pcie = cdns_pcie; > > pcie->mode = mode; > > pcie->linkdown_irq_regfield = data->linkdown_irq_regfield; > > > > @@ -426,28 +464,6 @@ static int j721e_pcie_probe(struct platform_device *pdev) > > > > switch (mode) { > > case PCI_MODE_RC: > > - if (!IS_ENABLED(CONFIG_PCIE_CADENCE_HOST)) { > > - ret = -ENODEV; > > - goto err_get_sync; > > - } > > - > > - bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc)); > > - if (!bridge) { > > - ret = -ENOMEM; > > - goto err_get_sync; > > - } > > - > > - if (!data->byte_access_allowed) > > - bridge->ops = &cdns_ti_pcie_host_ops; > > - rc = pci_host_bridge_priv(bridge); > > - rc->quirk_retrain_flag = data->quirk_retrain_flag; > > - rc->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag; > > - > > - cdns_pcie = &rc->pcie; > > - cdns_pcie->dev = dev; > > - cdns_pcie->ops = &j721e_pcie_ops; > > - pcie->cdns_pcie = cdns_pcie; > > - > > gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); > > if (IS_ERR(gpiod)) { > > ret = PTR_ERR(gpiod); > > @@ -497,23 +513,6 @@ static int j721e_pcie_probe(struct platform_device *pdev) > > > > break; > > case PCI_MODE_EP: > > - if (!IS_ENABLED(CONFIG_PCIE_CADENCE_EP)) { > > - ret = -ENODEV; > > - goto err_get_sync; > > - } > > - > > - ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL); > > - if (!ep) { > > - ret = -ENOMEM; > > - goto err_get_sync; > > - } > > - ep->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag; > > - > > - cdns_pcie = &ep->pcie; > > - cdns_pcie->dev = dev; > > - cdns_pcie->ops = &j721e_pcie_ops; > > - pcie->cdns_pcie = cdns_pcie; > > - > > ret = cdns_pcie_init_phy(dev, cdns_pcie); > > if (ret) { > > dev_err(dev, "Failed to init phy\n"); > > @@ -525,8 +524,6 @@ static int j721e_pcie_probe(struct platform_device *pdev) > > goto err_pcie_setup; > > > > break; > > - default: > > - dev_err(dev, "INVALID device type %d\n", mode); > > } > > > > return 0; > > > > -- > greets > -- > Christian Gmeiner, MSc > > https://christian-gmeiner.info/privacypolicy
diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c index 489586a4cdc7..cd43d1898482 100644 --- a/drivers/pci/controller/cadence/pci-j721e.c +++ b/drivers/pci/controller/cadence/pci-j721e.c @@ -51,10 +51,11 @@ enum link_status { #define MAX_LANES 2 struct j721e_pcie { - struct cdns_pcie *cdns_pcie; + struct device *dev; struct clk *refclk; u32 mode; u32 num_lanes; + struct cdns_pcie *cdns_pcie; void __iomem *user_cfg_base; void __iomem *intd_cfg_base; u32 linkdown_irq_regfield; @@ -98,7 +99,7 @@ static inline void j721e_pcie_intd_writel(struct j721e_pcie *pcie, u32 offset, static irqreturn_t j721e_pcie_link_irq_handler(int irq, void *priv) { struct j721e_pcie *pcie = priv; - struct device *dev = pcie->cdns_pcie->dev; + struct device *dev = pcie->dev; u32 reg; reg = j721e_pcie_intd_readl(pcie, STATUS_REG_SYS_2); @@ -164,7 +165,7 @@ static const struct cdns_pcie_ops j721e_pcie_ops = { static int j721e_pcie_set_mode(struct j721e_pcie *pcie, struct regmap *syscon, unsigned int offset) { - struct device *dev = pcie->cdns_pcie->dev; + struct device *dev = pcie->dev; u32 mask = J721E_MODE_RC; u32 mode = pcie->mode; u32 val = 0; @@ -183,7 +184,7 @@ static int j721e_pcie_set_mode(struct j721e_pcie *pcie, struct regmap *syscon, static int j721e_pcie_set_link_speed(struct j721e_pcie *pcie, struct regmap *syscon, unsigned int offset) { - struct device *dev = pcie->cdns_pcie->dev; + struct device *dev = pcie->dev; struct device_node *np = dev->of_node; int link_speed; u32 val = 0; @@ -204,7 +205,7 @@ static int j721e_pcie_set_link_speed(struct j721e_pcie *pcie, static int j721e_pcie_set_lane_count(struct j721e_pcie *pcie, struct regmap *syscon, unsigned int offset) { - struct device *dev = pcie->cdns_pcie->dev; + struct device *dev = pcie->dev; u32 lanes = pcie->num_lanes; u32 val = 0; int ret; @@ -219,7 +220,7 @@ static int j721e_pcie_set_lane_count(struct j721e_pcie *pcie, static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie) { - struct device *dev = pcie->cdns_pcie->dev; + struct device *dev = pcie->dev; struct device_node *node = dev->of_node; struct of_phandle_args args; unsigned int offset = 0; @@ -376,6 +377,7 @@ static int j721e_pcie_probe(struct platform_device *pdev) if (!pcie) return -ENOMEM; + pcie->dev = dev; pcie->mode = mode; pcie->linkdown_irq_regfield = data->linkdown_irq_regfield;
This reverts commit 19e863828acf6d8ac8475ba1fd93c0fe17fdc4ef. Fixes the following oops: Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010 Internal error: Oops: 96000004 [#1] PREEMPT SMP Modules linked in: CPU: 1 PID: 7 Comm: kworker/u4:0 Not tainted 5.17.0-rc1-00086-ge38b27816fea-dirty #71 Hardware name: CPE0108 (DT) Workqueue: events_unbound deferred_probe_work_func pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : j721e_pcie_probe+0x184/0x600 lr : j721e_pcie_probe+0x170/0x600 sp : ffff80000957bae0 x29: ffff80000957bae0 x28: ffff800009357000 x27: ffff00000000c078 x26: ffff00003fe047a8 x25: 0000000000000000 x24: ffff0000000f5280 x23: ffff800008c98f78 x22: ffff800008f90ff0 x21: ffff000000231410 x20: ffff000002ef2780 x19: 0000000000000021 x18: 0000000000000001 x17: 0000000000000000 x16: 0000000000058c00 x15: ffffffffffffffff x14: ffffffffffffffff x13: 0000000000000010 x12: 0101010101010101 x11: 0000000000000040 x10: ffff8000093e06c8 x9 : ffff8000093e06c0 x8 : ffff000000400270 x7 : 0000000000000000 x6 : ffff000000231590 x5 : ffff80000957b9e0 x4 : 0000000000000000 x3 : ffff0000002314f4 x2 : 0000000000000000 x1 : ffff0000000f5280 x0 : 0000000000000000 Call trace: j721e_pcie_probe+0x184/0x600 platform_probe+0x68/0xe0 really_probe+0x144/0x320 __driver_probe_device+0xc4/0xe0 driver_probe_device+0x7c/0x110 __device_attach_driver+0x90/0xe0 bus_for_each_drv+0x78/0xd0 __device_attach+0xf0/0x150 device_initial_probe+0x14/0x20 bus_probe_device+0x9c/0xb0 deferred_probe_work_func+0x88/0xc0 process_one_work+0x1bc/0x340 worker_thread+0x1f8/0x420 kthread+0x110/0x120 ret_from_fork+0x10/0x20 Code: f9400280 a90573fb d0005396 913fc2d6 (f9400800) Fixes: 19e863828acf ("PCI: j721e: Drop redundant struct device *") Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com> --- drivers/pci/controller/cadence/pci-j721e.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)