| Message ID | 20241008061916.313517-1-bingbu.cao@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] media: ipu6: fix the wrong type cast and 64-bit division | expand |
On Tue, Oct 08, 2024 at 02:19:15PM +0800, bingbu.cao@intel.com wrote: > From: Bingbu Cao <bingbu.cao@intel.com> > > This patch fixes the build errors with `i386-allmodconfig`, the > errors are caused by wrong type cast and 64-bit division. Thanks for the change, my comments below. ... > /* Shared structure between driver and FW - do not modify */ > struct ipu6_fw_sys_queue { > - u64 host_address; > + uintptr_t host_address; Okay, in the given semantic this probably should be phys_addr_t. BUT, is this address somehow is going to be used by IPU6 hardware? If "yes", the type shall not be changed. Looking at types used I hope the answer is "no", otherwise the types in the structures should be properly choose WRT endianess (and what __packed is doing here? Is it part of the protocol?). > u32 vied_address; > u32 size; > u32 token_size; > @@ -40,7 +40,7 @@ struct ipu6_fw_sys_queue { > } __packed; > > struct ipu6_fw_sys_queue_res { > - u64 host_address; > + uintptr_t host_address; Ditto. > u32 vied_address; > u32 reg; > } __packed; ... > - dev_dbg(dev, "write: reg 0x%lx = data 0x%x", base + addr - isys_base, > - data); > + dev_dbg(dev, "write: reg 0x%lx = data 0x%x", > + (ulong)(base + addr - isys_base), data); No, one should use proper specifiers for this. And what the heck 'ulong' is? Where is it being defined? ... > - dev_dbg(dev, "read: reg 0x%lx = data 0x%x", base + addr - isys_base, > - data); > + dev_dbg(dev, "read: reg 0x%lx = data 0x%x", > + (ulong)(base + addr - isys_base), data); Ditto. ... > pg_offset = server_fw_addr - dma_addr; > - prog = (struct ipu6_cell_program *)((u64)isp->cpd_fw->data + pg_offset); > + prog = (struct ipu6_cell_program *)((uintptr_t)isp->cpd_fw->data + > + pg_offset); Side Q: What are the alignment requirements for the prog pointer?
Andy, Thanks for the comments. On 10/8/24 11:23 PM, Andy Shevchenko wrote: > On Tue, Oct 08, 2024 at 02:19:15PM +0800, bingbu.cao@intel.com wrote: >> From: Bingbu Cao <bingbu.cao@intel.com> >> >> This patch fixes the build errors with `i386-allmodconfig`, the >> errors are caused by wrong type cast and 64-bit division. > > Thanks for the change, my comments below. > > ... > >> /* Shared structure between driver and FW - do not modify */ >> struct ipu6_fw_sys_queue { >> - u64 host_address; >> + uintptr_t host_address; > > Okay, in the given semantic this probably should be phys_addr_t. > BUT, is this address somehow is going to be used by IPU6 hardware? > If "yes", the type shall not be changed. > > Looking at types used I hope the answer is "no", otherwise the types > in the structures should be properly choose WRT endianess (and what > __packed is doing here? Is it part of the protocol?). You are correct, the type here should not be changed. > >> u32 vied_address; >> u32 size; >> u32 token_size; >> @@ -40,7 +40,7 @@ struct ipu6_fw_sys_queue { >> } __packed; >> >> struct ipu6_fw_sys_queue_res { >> - u64 host_address; >> + uintptr_t host_address; > > Ditto. > >> u32 vied_address; >> u32 reg; >> } __packed; > > ... > >> - dev_dbg(dev, "write: reg 0x%lx = data 0x%x", base + addr - isys_base, >> - data); >> + dev_dbg(dev, "write: reg 0x%lx = data 0x%x", >> + (ulong)(base + addr - isys_base), data); > > No, one should use proper specifiers for this. And what the heck 'ulong' is? > Where is it being defined? > > ... > >> - dev_dbg(dev, "read: reg 0x%lx = data 0x%x", base + addr - isys_base, >> - data); >> + dev_dbg(dev, "read: reg 0x%lx = data 0x%x", >> + (ulong)(base + addr - isys_base), data); > > Ditto. > > ... > >> pg_offset = server_fw_addr - dma_addr; >> - prog = (struct ipu6_cell_program *)((u64)isp->cpd_fw->data + pg_offset); >> + prog = (struct ipu6_cell_program *)((uintptr_t)isp->cpd_fw->data + >> + pg_offset); > > Side Q: What are the alignment requirements for the prog pointer? It should be 64 bytes alignment. >
On Wed, Oct 09, 2024 at 01:06:32PM +0800, Bingbu Cao wrote: > On 10/8/24 11:23 PM, Andy Shevchenko wrote: > > On Tue, Oct 08, 2024 at 02:19:15PM +0800, bingbu.cao@intel.com wrote: > >> From: Bingbu Cao <bingbu.cao@intel.com> ... > >> /* Shared structure between driver and FW - do not modify */ > >> struct ipu6_fw_sys_queue { > >> - u64 host_address; > >> + uintptr_t host_address; > > > > Okay, in the given semantic this probably should be phys_addr_t. > > BUT, is this address somehow is going to be used by IPU6 hardware? > > If "yes", the type shall not be changed. > > > > Looking at types used I hope the answer is "no", otherwise the types > > in the structures should be properly choose WRT endianess (and what > > __packed is doing here? Is it part of the protocol?). > > You are correct, the type here should not be changed. Okay, so perhaps for now we can amend the code to work around 32-bit compilation issues while leaving the type the same. Ideally this struct should be modified to follow endianess (all those types with __le prefixes), but that is another story. > >> u32 vied_address; > >> u32 size; > >> u32 token_size; > >> } __packed; ... > > Side Q: What are the alignment requirements for the prog pointer? > > It should be 64 bytes alignment. Okay, so in this case it's good to cast like this. Perhaps a comment should be added at some point, because it sounds like I already asked the very same question a while ago.
diff --git a/drivers/media/pci/intel/ipu6/ipu6-cpd.c b/drivers/media/pci/intel/ipu6/ipu6-cpd.c index 715b21ab4b8e..9b46b91cfc1a 100644 --- a/drivers/media/pci/intel/ipu6/ipu6-cpd.c +++ b/drivers/media/pci/intel/ipu6/ipu6-cpd.c @@ -43,9 +43,9 @@ * 63:56 55 54:48 47:32 31:24 23:0 * Rsvd Rsvd Type Version Rsvd Size */ -#define PKG_DIR_SIZE_MASK GENMASK(23, 0) -#define PKG_DIR_VERSION_MASK GENMASK(47, 32) -#define PKG_DIR_TYPE_MASK GENMASK(54, 48) +#define PKG_DIR_SIZE_MASK GENMASK_ULL(23, 0) +#define PKG_DIR_VERSION_MASK GENMASK_ULL(47, 32) +#define PKG_DIR_TYPE_MASK GENMASK_ULL(54, 48) static inline const struct ipu6_cpd_ent *ipu6_cpd_get_entry(const void *cpd, u8 idx) diff --git a/drivers/media/pci/intel/ipu6/ipu6-fw-com.c b/drivers/media/pci/intel/ipu6/ipu6-fw-com.c index 0b33fe9e703d..6da2aca69bd3 100644 --- a/drivers/media/pci/intel/ipu6/ipu6-fw-com.c +++ b/drivers/media/pci/intel/ipu6/ipu6-fw-com.c @@ -30,7 +30,7 @@ /* Shared structure between driver and FW - do not modify */ struct ipu6_fw_sys_queue { - u64 host_address; + uintptr_t host_address; u32 vied_address; u32 size; u32 token_size; @@ -40,7 +40,7 @@ struct ipu6_fw_sys_queue { } __packed; struct ipu6_fw_sys_queue_res { - u64 host_address; + uintptr_t host_address; u32 vied_address; u32 reg; } __packed; @@ -242,7 +242,7 @@ void *ipu6_fw_com_prepare(struct ipu6_fw_com_cfg *cfg, /* initialize input queues */ offset += specific_size; res.reg = SYSCOM_QPR_BASE_REG; - res.host_address = (u64)(ctx->dma_buffer + offset); + res.host_address = (uintptr_t)(ctx->dma_buffer + offset); res.vied_address = ctx->dma_addr + offset; for (i = 0; i < cfg->num_input_queues; i++) ipu6_sys_queue_init(ctx->input_queue + i, @@ -251,7 +251,7 @@ void *ipu6_fw_com_prepare(struct ipu6_fw_com_cfg *cfg, /* initialize output queues */ offset += sizeinput; - res.host_address = (u64)(ctx->dma_buffer + offset); + res.host_address = (uintptr_t)(ctx->dma_buffer + offset); res.vied_address = ctx->dma_addr + offset; for (i = 0; i < cfg->num_output_queues; i++) { ipu6_sys_queue_init(ctx->output_queue + i, diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-dwc-phy.c b/drivers/media/pci/intel/ipu6/ipu6-isys-dwc-phy.c index 1715275e6776..1cfc9a45bcea 100644 --- a/drivers/media/pci/intel/ipu6/ipu6-isys-dwc-phy.c +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-dwc-phy.c @@ -67,8 +67,8 @@ static void dwc_dphy_write(struct ipu6_isys *isys, u32 phy_id, u32 addr, void __iomem *isys_base = isys->pdata->base; void __iomem *base = isys_base + IPU6_DWC_DPHY_BASE(phy_id); - dev_dbg(dev, "write: reg 0x%lx = data 0x%x", base + addr - isys_base, - data); + dev_dbg(dev, "write: reg 0x%lx = data 0x%x", + (ulong)(base + addr - isys_base), data); writel(data, base + addr); } @@ -80,8 +80,8 @@ static u32 dwc_dphy_read(struct ipu6_isys *isys, u32 phy_id, u32 addr) u32 data; data = readl(base + addr); - dev_dbg(dev, "read: reg 0x%lx = data 0x%x", base + addr - isys_base, - data); + dev_dbg(dev, "read: reg 0x%lx = data 0x%x", + (ulong)(base + addr - isys_base), data); return data; } diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys.c b/drivers/media/pci/intel/ipu6/ipu6-isys.c index c4aff2e2009b..51c1a567eff8 100644 --- a/drivers/media/pci/intel/ipu6/ipu6-isys.c +++ b/drivers/media/pci/intel/ipu6/ipu6-isys.c @@ -576,7 +576,7 @@ void update_watermark_setting(struct ipu6_isys *isys) } enable_iwake(isys, true); - calc_fill_time_us = max_sram_size / isys_pb_datarate_mbs; + calc_fill_time_us = div64_u64(max_sram_size, isys_pb_datarate_mbs); if (isys->pdata->ipdata->enhanced_iwake) { ltr = isys->pdata->ipdata->ltr; diff --git a/drivers/media/pci/intel/ipu6/ipu6.c b/drivers/media/pci/intel/ipu6/ipu6.c index 7fb707d35309..797d11d1d3b1 100644 --- a/drivers/media/pci/intel/ipu6/ipu6.c +++ b/drivers/media/pci/intel/ipu6/ipu6.c @@ -247,7 +247,8 @@ ipu6_pkg_dir_configure_spc(struct ipu6_device *isp, dma_addr = sg_dma_address(isp->psys->fw_sgt.sgl); pg_offset = server_fw_addr - dma_addr; - prog = (struct ipu6_cell_program *)((u64)isp->cpd_fw->data + pg_offset); + prog = (struct ipu6_cell_program *)((uintptr_t)isp->cpd_fw->data + + pg_offset); spc_base = base + prog->regs_addr; if (spc_base != (base + hw_variant->spc_offset)) dev_warn(&isp->pdev->dev,