From: Bjorn Helgaas <helgaas@kernel.org>
To: Frank Li <frank.li@nxp.com>
Cc: "mani@kernel.org" <mani@kernel.org>,
"allenbh@gmail.com" <allenbh@gmail.com>,
"bhelgaas@google.com" <bhelgaas@google.com>,
"dave.jiang@intel.com" <dave.jiang@intel.com>,
"imx@lists.linux.dev" <imx@lists.linux.dev>,
"jdmason@kudzu.us" <jdmason@kudzu.us>,
"kw@linux.com" <kw@linux.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
"lpieralisi@kernel.org" <lpieralisi@kernel.org>,
"ntb@lists.linux.dev" <ntb@lists.linux.dev>
Subject: Re: [EXT] Re: [PATCH v16 7/7] PCI: endpoint: pci-epf-vntb: fix sparse build warning at ntb->reg
Date: Wed, 14 Dec 2022 05:35:12 -0600 [thread overview]
Message-ID: <20221214113512.GA247043@bhelgaas> (raw)
In-Reply-To: <HE1PR0401MB2331E04D41F5EF9F6F1E326288E09@HE1PR0401MB2331.eurprd04.prod.outlook.com>
On Wed, Dec 14, 2022 at 12:49:15AM +0000, Frank Li wrote:
> >
> > On Wed, Nov 02, 2022 at 10:10:14AM -0400, Frank Li wrote:
> > > From: Frank Li <frank.li@nxp.com>
> > >
> > > pci-epf-vntb.c:1128:33: sparse: expected void [noderef] __iomem
> > *base
> > > pci-epf-vntb.c:1128:33: sparse: got struct epf_ntb_ctrl *reg
> > >
> > > Add __iomem type convert in vntb_epf_peer_spad_read() and
> > > vntb_epf_peer_spad_write().
> >
> > I don't understand all the bits and pieces here, but I'm a little
> > dubious about adding all these "(void __iomem *)"casts. There are
> > very few of them in drivers/pci/, and I doubt this driver is so unique
> > that it needs them.
>
> sparse compiler report warning without cast. I write it at commit message.
As a matter of fact, I did read your commit message. My point is that
I don't think littering the code with casts is the best solution. I
wrote more details below; please read the entire email.
> > > @@ -1121,7 +1121,7 @@ static u32 vntb_epf_spad_read(struct ntb_dev
> > *ndev, int idx)
> > > struct epf_ntb *ntb = ntb_ndev(ndev);
> > > int off = ntb->reg->spad_offset, ct = ntb->reg->spad_count *
> > sizeof(u32);
> > > u32 val;
> > > - void __iomem *base = ntb->reg;
> > > + void __iomem *base = (void __iomem *)ntb->reg;
> > >
> > > val = readl(base + off + ct + idx * sizeof(u32));
> > > return val;
> > > @@ -1132,7 +1132,7 @@ static int vntb_epf_spad_write(struct ntb_dev
> > *ndev, int idx, u32 val)
> > > struct epf_ntb *ntb = ntb_ndev(ndev);
> > > struct epf_ntb_ctrl *ctrl = ntb->reg;
> > > int off = ctrl->spad_offset, ct = ctrl->spad_count * sizeof(u32);
> > > - void __iomem *base = ntb->reg;
> > > + void __iomem *base = (void __iomem *)ntb->reg;
> > >
> > > writel(val, base + off + ct + idx * sizeof(u32));
> >
> > These things look gratuitously different to begin with:
> >
> > int off = ntb->reg->spad_offset, ct = ntb->reg->spad_count * sizeof(u32);
> > int off = ctrl->spad_offset, ct = ctrl->spad_count * sizeof(u32);
> >
> > They're doing the same thing, and they should do it the same way.
> >
> > Since db_data[] and db_offset[] are never referenced except to be
> > initialized to zero, I'm guessing the point of vntb_epf_spad_read()
> > and vntb_epf_spad_write() is to read/write things in those arrays?
> >
> > You access other things in ntb->reg directly by dereferencing a
> > pointer, e.g.,
> >
> > ntb->reg->link_status |= LINK_STATUS_UP;
> > addr = ntb->reg->addr;
> > ctrl->command_status = COMMAND_STATUS_OK;
> >
> > Why don't you just compute the appropriate *index* and access the
> > array directly instead of using readl() and writel()?
> >
> > Bjorn
next prev parent reply other threads:[~2022-12-14 11:35 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-02 14:10 [PATCH v16 0/7] pci-epf-vntb clean up Frank Li
2022-11-02 14:10 ` [PATCH v16 1/7] PCI: endpoint: pci-epf-vntb: clean up kernel_doc warning Frank Li
2022-11-02 14:10 ` [PATCH v16 2/7] PCI: endpoint: pci-epf-vntb: fix indentation of the struct epf_ntb_ctrl Frank Li
2022-11-02 14:10 ` [PATCH v16 3/7] PCI: endpoint: pci-epf-vntb: fix call pci_epc_mem_free_addr() at err path Frank Li
2022-11-02 14:10 ` [PATCH v16 4/7] PCI: endpoint: pci-epf-vntb: remove unused field epf_db_phy Frank Li
2022-11-10 15:28 ` Lorenzo Pieralisi
2022-11-11 2:39 ` [EXT] " Frank Li
2022-11-11 10:55 ` Lorenzo Pieralisi
2022-11-15 20:49 ` Frank Li
2022-11-02 14:10 ` [PATCH v16 5/7] PCI: endpoint: pci-epf-vntb: replace hardcode 4 with sizeof(u32) Frank Li
2022-11-02 14:10 ` [PATCH v16 6/7] PCI: endpoint: pci-epf-vntb: fix sparse build warning at epf_db Frank Li
2022-11-02 14:10 ` [PATCH v16 7/7] PCI: endpoint: pci-epf-vntb: fix sparse build warning at ntb->reg Frank Li
2022-12-14 0:26 ` Bjorn Helgaas
2022-12-14 0:49 ` [EXT] " Frank Li
2022-12-14 11:35 ` Bjorn Helgaas [this message]
2022-12-14 15:24 ` Frank Li
2022-11-02 23:03 ` [PATCH v16 0/7] pci-epf-vntb clean up Bjorn Helgaas
2022-11-03 0:59 ` [EXT] " Frank Li
2022-11-23 17:11 ` Lorenzo Pieralisi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221214113512.GA247043@bhelgaas \
--to=helgaas@kernel.org \
--cc=allenbh@gmail.com \
--cc=bhelgaas@google.com \
--cc=dave.jiang@intel.com \
--cc=frank.li@nxp.com \
--cc=imx@lists.linux.dev \
--cc=jdmason@kudzu.us \
--cc=kw@linux.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lpieralisi@kernel.org \
--cc=mani@kernel.org \
--cc=ntb@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).