| Message ID | 20180201161119.3852-4-niklas.cassel@axis.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Thu, Feb 1, 2018 at 6:11 PM, Niklas Cassel <niklas.cassel@axis.com> wrote:
include/linux/sizes.h:
+SZ_4G 0x100000000ULL
> + if (size > 0x100000000ULL) {
#include <linux/sizes.h>
if (size > SZ_4G) {
?
On Thursday, February 1, 2018 1:58 PM, Andy Shevchenko wrote: > > On Thu, Feb 1, 2018 at 6:11 PM, Niklas Cassel <niklas.cassel@axis.com> > wrote: > > include/linux/sizes.h: > > +SZ_4G 0x100000000ULL > > > + if (size > 0x100000000ULL) { > > #include <linux/sizes.h> > > if (size > SZ_4G) { I like this one for the readability. Thank you. Best regards, Jingoo Han > > ? > > -- > With Best Regards, > Andy Shevchenko
On Thu, Feb 01, 2018 at 02:00:40PM -0500, Jingoo Han wrote: > On Thursday, February 1, 2018 1:58 PM, Andy Shevchenko wrote: > > > > On Thu, Feb 1, 2018 at 6:11 PM, Niklas Cassel <niklas.cassel@axis.com> > > wrote: > > > > include/linux/sizes.h: > > > > +SZ_4G 0x100000000ULL > > > > > + if (size > 0x100000000ULL) { > > > > #include <linux/sizes.h> > > > > if (size > SZ_4G) { > > I like this one for the readability. > Thank you. > I liked it too, however both variants if (size > 0x100000000ULL) { if (size > SZ_4G) { result in: drivers/pci/dwc/pcie-designware-ep.c:131:11: warning: comparison is always false due to limited range of data type [-Wtype-limits] when compiling with W=1 on a platform with 32-bit size_t. The annoying thing here is that a BAR can be 64-bit, yet the parameter size is defined as a size_t, so the error will only show on 32-bit and not on 64-bit. What do you think about: if (upper_32_bits(size)) { dev_err(pci->dev, "can't handle BAR larger than 4GB\n"); return -EINVAL; } That should compile without warnings for both 32-bit size_t and 64-bit size_t. Regards, Niklas
On Mon, Feb 5, 2018 at 6:25 PM, Niklas Cassel <niklas.cassel@axis.com> wrote: > On Thu, Feb 01, 2018 at 02:00:40PM -0500, Jingoo Han wrote: >> On Thursday, February 1, 2018 1:58 PM, Andy Shevchenko wrote: >> > >> > On Thu, Feb 1, 2018 at 6:11 PM, Niklas Cassel <niklas.cassel@axis.com> >> > wrote: >> > >> > include/linux/sizes.h: >> > >> > +SZ_4G 0x100000000ULL >> > >> > > + if (size > 0x100000000ULL) { >> > >> > #include <linux/sizes.h> >> > >> > if (size > SZ_4G) { >> >> I like this one for the readability. >> Thank you. >> > > I liked it too, however both variants > > if (size > 0x100000000ULL) { > > if (size > SZ_4G) { > > result in: > > drivers/pci/dwc/pcie-designware-ep.c:131:11: warning: > comparison is always false due to limited range of data type [-Wtype-limits] > > when compiling with W=1 on a platform with 32-bit size_t. > > > The annoying thing here is that a BAR can be 64-bit, > yet the parameter size is defined as a size_t, > so the error will only show on 32-bit and not on 64-bit. Oh, indeed. And it looks moving to u64 or alike is not a solution (because if would not describe real hardware in that case). > What do you think about: > if (upper_32_bits(size)) { > dev_err(pci->dev, "can't handle BAR larger than 4GB\n"); > return -EINVAL; > } > > That should compile without warnings for both > 32-bit size_t and 64-bit size_t. Can you derive some helper based on the code in __pci_read_base() code?
On Tue, Feb 06, 2018 at 09:38:09PM +0200, Andy Shevchenko wrote: > On Mon, Feb 5, 2018 at 6:25 PM, Niklas Cassel <niklas.cassel@axis.com> wrote: > > On Thu, Feb 01, 2018 at 02:00:40PM -0500, Jingoo Han wrote: > >> On Thursday, February 1, 2018 1:58 PM, Andy Shevchenko wrote: > >> > > >> > On Thu, Feb 1, 2018 at 6:11 PM, Niklas Cassel <niklas.cassel@axis.com> > >> > wrote: > >> > > >> > include/linux/sizes.h: > >> > > >> > +SZ_4G 0x100000000ULL > >> > > >> > > + if (size > 0x100000000ULL) { > >> > > >> > #include <linux/sizes.h> > >> > > >> > if (size > SZ_4G) { > >> > >> I like this one for the readability. > >> Thank you. > >> > > > > I liked it too, however both variants > > > > if (size > 0x100000000ULL) { > > > > if (size > SZ_4G) { > > > > result in: > > > > drivers/pci/dwc/pcie-designware-ep.c:131:11: warning: > > comparison is always false due to limited range of data type [-Wtype-limits] > > > > when compiling with W=1 on a platform with 32-bit size_t. > > > > > > The annoying thing here is that a BAR can be 64-bit, > > yet the parameter size is defined as a size_t, > > so the error will only show on 32-bit and not on 64-bit. > > Oh, indeed. And it looks moving to u64 or alike is not a solution > (because if would not describe real hardware in that case). > > > What do you think about: > > > if (upper_32_bits(size)) { > > dev_err(pci->dev, "can't handle BAR larger than 4GB\n"); > > return -EINVAL; > > } > > > > That should compile without warnings for both > > 32-bit size_t and 64-bit size_t. > > Can you derive some helper based on the code in __pci_read_base() code? Hello Andy, I guess that would be possible, however I think that simply checking upper_32_bits(size) is simpler. If someone is ever to fix dw_pcie_ep_set_bar() so that it works with 64-bit BARs, the function will probably need to check upper_32_bits(size) anyway. Regards, Niklas
diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c index 3a6feeff5f5b..4a0085ead1e3 100644 --- a/drivers/pci/dwc/pcie-designware-ep.c +++ b/drivers/pci/dwc/pcie-designware-ep.c @@ -126,6 +126,11 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, enum dw_pcie_as_type as_type; u32 reg = PCI_BASE_ADDRESS_0 + (4 * bar); + if (size > 0x100000000ULL) { + dev_err(pci->dev, "can't handle BAR larger than 4GB\n"); + return -EINVAL; + } + if (!(flags & PCI_BASE_ADDRESS_SPACE)) as_type = DW_PCIE_AS_MEM; else
pci_epc_set_bar() can be called with flag PCI_BASE_ADDRESS_MEM_TYPE_64, and can thus request a BAR size larger than 4 GB. However, the pcie-designware-ep.c driver currently doesn't handle BAR sizes larger than 4 GB. (Since we are only writing the BAR_mask[x] register and not the BAR_mask[x+1] register.) For now, return an error when requesting a BAR size larger than 4 GB. Signed-off-by: Niklas Cassel <niklas.cassel@axis.com> --- drivers/pci/dwc/pcie-designware-ep.c | 5 +++++ 1 file changed, 5 insertions(+)