| Message ID | 20200924190623.3251c2ac@xhacker.debian |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: dwc: improve msi handling | expand |
On 2020-09-24 12:06, Jisheng Zhang wrote: > We need to check alloc_page() succeed or not before continuing. > > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > --- > drivers/pci/controller/dwc/pcie-designware-host.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c > b/drivers/pci/controller/dwc/pcie-designware-host.c > index 0a19de946351..9e04e8ef3aa4 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -303,6 +303,11 @@ void dw_pcie_msi_init(struct pcie_port *pp) > u64 msi_target; > > pp->msi_page = alloc_page(GFP_KERNEL); > + if (!pp->msi_page) { > + dev_err(dev, "Failed to alloc MSI page\n"); A failing allocation will already scream, so there is no need to add insult to injury. More importantly, what is this MSI page ever used for? If I remember well, this is just a random address that never gets written to. So why do we allocate a page here? Why do we bother with this DMA mapping? M. -- Who you jivin' with that Cosmik Debris?
Hi Marc, On Tue, 29 Sep 2020 18:29:52 +0100 Marc Zyngier wrote: > > > On 2020-09-24 12:06, Jisheng Zhang wrote: > > We need to check alloc_page() succeed or not before continuing. > > > > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > > --- > > drivers/pci/controller/dwc/pcie-designware-host.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c > > b/drivers/pci/controller/dwc/pcie-designware-host.c > > index 0a19de946351..9e04e8ef3aa4 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > > @@ -303,6 +303,11 @@ void dw_pcie_msi_init(struct pcie_port *pp) > > u64 msi_target; > > > > pp->msi_page = alloc_page(GFP_KERNEL); > > + if (!pp->msi_page) { > > + dev_err(dev, "Failed to alloc MSI page\n"); > > A failing allocation will already scream, so there is no need to > add insult to injury. > > More importantly, what is this MSI page ever used for? If I remember > well, this is just a random address that never gets written to. > > So why do we allocate a page here? Why do we bother with this DMA > mapping? > Ard and Rob also pointed out that there's no need to allocate a page, instead, we could use an address in the driver data for MSI address. So I refactored the patches and verified this solution works fine. Could you please review the V5? Thanks
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index 0a19de946351..9e04e8ef3aa4 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -303,6 +303,11 @@ void dw_pcie_msi_init(struct pcie_port *pp) u64 msi_target; pp->msi_page = alloc_page(GFP_KERNEL); + if (!pp->msi_page) { + dev_err(dev, "Failed to alloc MSI page\n"); + return; + } + pp->msi_data = dma_map_page(dev, pp->msi_page, 0, PAGE_SIZE, DMA_FROM_DEVICE); if (dma_mapping_error(dev, pp->msi_data)) {
We need to check alloc_page() succeed or not before continuing. Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> --- drivers/pci/controller/dwc/pcie-designware-host.c | 5 +++++ 1 file changed, 5 insertions(+)