| Message ID | 20241014105912.3207374-34-ryan.roberts@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Boot-time page size selection for arm64 | expand |
Hello Ryan, While I realize that this has not always been consistent, please prefix the subject with "ata: ", so that it becomes "ata: sata_sil24: ". On Mon, Oct 14, 2024 at 11:58:41AM +0100, Ryan Roberts wrote: > To prepare for supporting boot-time page size selection, refactor code > to remove assumptions about PAGE_SIZE being compile-time constant. Code > intended to be equivalent when compile-time page size is active. > > Convert "struct sil24_ata_block" and "struct sil24_atapi_block" to use a > flexible array member for their sge[] array. The previous static size of > SIL24_MAX_SGE depends on PAGE_SIZE so doesn't work for boot-time page > size. > > Wrap global variables that are initialized with PAGE_SIZE derived values > using DEFINE_GLOBAL_PAGE_SIZE_VAR() so their initialization can be > deferred for boot-time page size builds. > > Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> > --- > > ***NOTE*** > Any confused maintainers may want to read the cover note here for context: > https://lore.kernel.org/all/20241014105514.3206191-1-ryan.roberts@arm.com/ > > drivers/ata/sata_sil24.c | 46 +++++++++++++++++++--------------------- > 1 file changed, 22 insertions(+), 24 deletions(-) > > diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c > index 72c03cbdaff43..85c6382976626 100644 > --- a/drivers/ata/sata_sil24.c > +++ b/drivers/ata/sata_sil24.c > @@ -42,26 +42,25 @@ struct sil24_sge { > __le32 flags; > }; > > +/* > + * sil24 fetches in chunks of 64bytes. The first block > + * contains the PRB and two SGEs. From the second block, it's > + * consisted of four SGEs and called SGT. Calculate the > + * number of SGTs that fit into one page. > + */ > +#define SIL24_PRB_SZ (sizeof(struct sil24_prb) + 2 * sizeof(struct sil24_sge)) > +#define SIL24_MAX_SGT ((PAGE_SIZE - SIL24_PRB_SZ) / (4 * sizeof(struct sil24_sge))) > + > +/* > + * This will give us one unused SGEs for ATA. This extra SGE > + * will be used to store CDB for ATAPI devices. > + */ > +#define SIL24_MAX_SGE (4 * SIL24_MAX_SGT + 1) > > enum { > SIL24_HOST_BAR = 0, > SIL24_PORT_BAR = 2, > > - /* sil24 fetches in chunks of 64bytes. The first block > - * contains the PRB and two SGEs. From the second block, it's > - * consisted of four SGEs and called SGT. Calculate the > - * number of SGTs that fit into one page. > - */ > - SIL24_PRB_SZ = sizeof(struct sil24_prb) > - + 2 * sizeof(struct sil24_sge), > - SIL24_MAX_SGT = (PAGE_SIZE - SIL24_PRB_SZ) > - / (4 * sizeof(struct sil24_sge)), > - > - /* This will give us one unused SGEs for ATA. This extra SGE > - * will be used to store CDB for ATAPI devices. > - */ > - SIL24_MAX_SGE = 4 * SIL24_MAX_SGT + 1, > - > /* > * Global controller registers (128 bytes @ BAR0) > */ > @@ -244,13 +243,13 @@ enum { > > struct sil24_ata_block { > struct sil24_prb prb; > - struct sil24_sge sge[SIL24_MAX_SGE]; > + struct sil24_sge sge[]; > }; > > struct sil24_atapi_block { > struct sil24_prb prb; > u8 cdb[16]; > - struct sil24_sge sge[SIL24_MAX_SGE]; > + struct sil24_sge sge[]; > }; > > union sil24_cmd_block { > @@ -373,7 +372,7 @@ static struct pci_driver sil24_pci_driver = { > #endif > }; > > -static const struct scsi_host_template sil24_sht = { > +static DEFINE_GLOBAL_PAGE_SIZE_VAR_CONST(struct scsi_host_template, sil24_sht, { > __ATA_BASE_SHT(DRV_NAME), > .can_queue = SIL24_MAX_CMDS, > .sg_tablesize = SIL24_MAX_SGE, > @@ -382,7 +381,7 @@ static const struct scsi_host_template sil24_sht = { > .sdev_groups = ata_ncq_sdev_groups, > .change_queue_depth = ata_scsi_change_queue_depth, > .device_configure = ata_scsi_device_configure > -}; > +}); > > static struct ata_port_operations sil24_ops = { > .inherits = &sata_pmp_port_ops, > @@ -1193,7 +1192,7 @@ static int sil24_port_start(struct ata_port *ap) > struct device *dev = ap->host->dev; > struct sil24_port_priv *pp; > union sil24_cmd_block *cb; > - size_t cb_size = sizeof(*cb) * SIL24_MAX_CMDS; > + size_t cb_size = PAGE_SIZE * SIL24_MAX_CMDS; > dma_addr_t cb_dma; > > pp = devm_kzalloc(dev, sizeof(*pp), GFP_KERNEL); > @@ -1258,7 +1257,6 @@ static void sil24_init_controller(struct ata_host *host) > > static int sil24_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > { > - extern int __MARKER__sil24_cmd_block_is_sized_wrongly; > struct ata_port_info pi = sil24_port_info[ent->driver_data]; > const struct ata_port_info *ppi[] = { &pi, NULL }; > void __iomem * const *iomap; > @@ -1266,9 +1264,9 @@ static int sil24_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > int rc; > u32 tmp; > > - /* cause link error if sil24_cmd_block is sized wrongly */ > - if (sizeof(union sil24_cmd_block) != PAGE_SIZE) > - __MARKER__sil24_cmd_block_is_sized_wrongly = 1; > + /* union sil24_cmd_block must be PAGE_SIZE */ > + BUG_ON(struct_size_t(struct sil24_atapi_block, sge, SIL24_MAX_SGE) != PAGE_SIZE); > + BUG_ON(struct_size_t(struct sil24_ata_block, sge, SIL24_MAX_SGE) > PAGE_SIZE); > > ata_print_version_once(&pdev->dev, DRV_VERSION); > > -- > 2.43.0 > As you might know, there is an effort to annotate all flexible array members with their run-time size information, see commit: dd06e72e68bc ("Compiler Attributes: Add __counted_by macro") I haven't looked at the DEFINE_GLOBAL_PAGE_SIZE_VAR_CONST macro, but since sge[] now becomes a flexible array member, I think it would be nice if it would be possible to somehow use the __counted_by macro. Other than that, this looks good to me. Kind regards, Niklas
On 17/10/2024 10:09, Niklas Cassel wrote: > Hello Ryan, > > While I realize that this has not always been consistent, > please prefix the subject with "ata: ", so that it becomes > "ata: sata_sil24: ". Noted; I'll fix this in the next version. > > On Mon, Oct 14, 2024 at 11:58:41AM +0100, Ryan Roberts wrote: >> To prepare for supporting boot-time page size selection, refactor code >> to remove assumptions about PAGE_SIZE being compile-time constant. Code >> intended to be equivalent when compile-time page size is active. >> >> Convert "struct sil24_ata_block" and "struct sil24_atapi_block" to use a >> flexible array member for their sge[] array. The previous static size of >> SIL24_MAX_SGE depends on PAGE_SIZE so doesn't work for boot-time page >> size. >> >> Wrap global variables that are initialized with PAGE_SIZE derived values >> using DEFINE_GLOBAL_PAGE_SIZE_VAR() so their initialization can be >> deferred for boot-time page size builds. >> >> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> >> --- >> >> ***NOTE*** >> Any confused maintainers may want to read the cover note here for context: >> https://lore.kernel.org/all/20241014105514.3206191-1-ryan.roberts@arm.com/ >> >> drivers/ata/sata_sil24.c | 46 +++++++++++++++++++--------------------- >> 1 file changed, 22 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c >> index 72c03cbdaff43..85c6382976626 100644 >> --- a/drivers/ata/sata_sil24.c >> +++ b/drivers/ata/sata_sil24.c >> @@ -42,26 +42,25 @@ struct sil24_sge { >> __le32 flags; >> }; >> >> +/* >> + * sil24 fetches in chunks of 64bytes. The first block >> + * contains the PRB and two SGEs. From the second block, it's >> + * consisted of four SGEs and called SGT. Calculate the >> + * number of SGTs that fit into one page. >> + */ >> +#define SIL24_PRB_SZ (sizeof(struct sil24_prb) + 2 * sizeof(struct sil24_sge)) >> +#define SIL24_MAX_SGT ((PAGE_SIZE - SIL24_PRB_SZ) / (4 * sizeof(struct sil24_sge))) >> + >> +/* >> + * This will give us one unused SGEs for ATA. This extra SGE >> + * will be used to store CDB for ATAPI devices. >> + */ >> +#define SIL24_MAX_SGE (4 * SIL24_MAX_SGT + 1) >> >> enum { >> SIL24_HOST_BAR = 0, >> SIL24_PORT_BAR = 2, >> >> - /* sil24 fetches in chunks of 64bytes. The first block >> - * contains the PRB and two SGEs. From the second block, it's >> - * consisted of four SGEs and called SGT. Calculate the >> - * number of SGTs that fit into one page. >> - */ >> - SIL24_PRB_SZ = sizeof(struct sil24_prb) >> - + 2 * sizeof(struct sil24_sge), >> - SIL24_MAX_SGT = (PAGE_SIZE - SIL24_PRB_SZ) >> - / (4 * sizeof(struct sil24_sge)), >> - >> - /* This will give us one unused SGEs for ATA. This extra SGE >> - * will be used to store CDB for ATAPI devices. >> - */ >> - SIL24_MAX_SGE = 4 * SIL24_MAX_SGT + 1, >> - >> /* >> * Global controller registers (128 bytes @ BAR0) >> */ >> @@ -244,13 +243,13 @@ enum { >> >> struct sil24_ata_block { >> struct sil24_prb prb; >> - struct sil24_sge sge[SIL24_MAX_SGE]; >> + struct sil24_sge sge[]; >> }; >> >> struct sil24_atapi_block { >> struct sil24_prb prb; >> u8 cdb[16]; >> - struct sil24_sge sge[SIL24_MAX_SGE]; >> + struct sil24_sge sge[]; >> }; >> >> union sil24_cmd_block { >> @@ -373,7 +372,7 @@ static struct pci_driver sil24_pci_driver = { >> #endif >> }; >> >> -static const struct scsi_host_template sil24_sht = { >> +static DEFINE_GLOBAL_PAGE_SIZE_VAR_CONST(struct scsi_host_template, sil24_sht, { >> __ATA_BASE_SHT(DRV_NAME), >> .can_queue = SIL24_MAX_CMDS, >> .sg_tablesize = SIL24_MAX_SGE, >> @@ -382,7 +381,7 @@ static const struct scsi_host_template sil24_sht = { >> .sdev_groups = ata_ncq_sdev_groups, >> .change_queue_depth = ata_scsi_change_queue_depth, >> .device_configure = ata_scsi_device_configure >> -}; >> +}); >> >> static struct ata_port_operations sil24_ops = { >> .inherits = &sata_pmp_port_ops, >> @@ -1193,7 +1192,7 @@ static int sil24_port_start(struct ata_port *ap) >> struct device *dev = ap->host->dev; >> struct sil24_port_priv *pp; >> union sil24_cmd_block *cb; >> - size_t cb_size = sizeof(*cb) * SIL24_MAX_CMDS; >> + size_t cb_size = PAGE_SIZE * SIL24_MAX_CMDS; >> dma_addr_t cb_dma; >> >> pp = devm_kzalloc(dev, sizeof(*pp), GFP_KERNEL); >> @@ -1258,7 +1257,6 @@ static void sil24_init_controller(struct ata_host *host) >> >> static int sil24_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) >> { >> - extern int __MARKER__sil24_cmd_block_is_sized_wrongly; >> struct ata_port_info pi = sil24_port_info[ent->driver_data]; >> const struct ata_port_info *ppi[] = { &pi, NULL }; >> void __iomem * const *iomap; >> @@ -1266,9 +1264,9 @@ static int sil24_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) >> int rc; >> u32 tmp; >> >> - /* cause link error if sil24_cmd_block is sized wrongly */ >> - if (sizeof(union sil24_cmd_block) != PAGE_SIZE) >> - __MARKER__sil24_cmd_block_is_sized_wrongly = 1; >> + /* union sil24_cmd_block must be PAGE_SIZE */ >> + BUG_ON(struct_size_t(struct sil24_atapi_block, sge, SIL24_MAX_SGE) != PAGE_SIZE); >> + BUG_ON(struct_size_t(struct sil24_ata_block, sge, SIL24_MAX_SGE) > PAGE_SIZE); >> >> ata_print_version_once(&pdev->dev, DRV_VERSION); >> >> -- >> 2.43.0 >> > > As you might know, there is an effort to annotate all flexible array > members with their run-time size information, see commit: > dd06e72e68bc ("Compiler Attributes: Add __counted_by macro") I'm vaguely aware of it. But as I understand it, __counted_by() nominates another member in the struct which keeps the count? In this case, there is no such member, it's size is implicit based on the value of PAGE_SIZE. So I'm not sure if it's practical to use it here? > > I haven't looked at the DEFINE_GLOBAL_PAGE_SIZE_VAR_CONST macro, but since DEFINE_GLOBAL_PAGE_SIZE_VAR_CONST(), when doing a boot-time page size build, defers the initialization of the global variable to kernel init time, when PAGE_SIZE is known. Because SIL24_MAX_SGE is defined in terms of PAGE_SIZE, this deferral is required. > sge[] now becomes a flexible array member, I think it would be nice if it > would be possible to somehow use the __counted_by macro. > > Other than that, this looks good to me. Thanks for the review! > > > Kind regards, > Niklas
On Thu, Oct 17, 2024 at 01:42:22PM +0100, Ryan Roberts wrote: > On 17/10/2024 10:09, Niklas Cassel wrote: (snip) > > As you might know, there is an effort to annotate all flexible array > > members with their run-time size information, see commit: > > dd06e72e68bc ("Compiler Attributes: Add __counted_by macro") > > I'm vaguely aware of it. But as I understand it, __counted_by() nominates > another member in the struct which keeps the count? In this case, there is no > such member, it's size is implicit based on the value of PAGE_SIZE. So I'm not > sure if it's practical to use it here? Neither am I :) Perhaps some of the flexible array member experts like Kees Cook or Gustavo A. R. Silva could help us out here. Would it make sense to add another struct member and simply initialize it to PAGE_SIZE, in order to be able to use the __counted_by macro? > > > > > I haven't looked at the DEFINE_GLOBAL_PAGE_SIZE_VAR_CONST macro, but since > > DEFINE_GLOBAL_PAGE_SIZE_VAR_CONST(), when doing a boot-time page size build, > defers the initialization of the global variable to kernel init time, when > PAGE_SIZE is known. Because SIL24_MAX_SGE is defined in terms of PAGE_SIZE, this > deferral is required. > > > sge[] now becomes a flexible array member, I think it would be nice if it > > would be possible to somehow use the __counted_by macro. > >
On 17/10/2024 13:51, Niklas Cassel wrote: > On Thu, Oct 17, 2024 at 01:42:22PM +0100, Ryan Roberts wrote: >> On 17/10/2024 10:09, Niklas Cassel wrote: > > (snip) > >>> As you might know, there is an effort to annotate all flexible array >>> members with their run-time size information, see commit: >>> dd06e72e68bc ("Compiler Attributes: Add __counted_by macro") >> >> I'm vaguely aware of it. But as I understand it, __counted_by() nominates >> another member in the struct which keeps the count? In this case, there is no >> such member, it's size is implicit based on the value of PAGE_SIZE. So I'm not >> sure if it's practical to use it here? > > Neither am I :) > > Perhaps some of the flexible array member experts like > Kees Cook or Gustavo A. R. Silva could help us out here. The GCC feature request is clear that it is explicitly to mark a member as the count variable: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108896 But, yes, would be good to hear from Kees or Gustavo if there is an alternative mechanism for what we are doing here. > > Would it make sense to add another struct member and simply initialize > it to PAGE_SIZE, in order to be able to use the __counted_by macro? I guess that _could_ be done. But the way the driver is currently structured takes the sge array pointer and passes that around for DMA, so I think the value of this tag within the struct would be lost anyway. It would also require reducing the number of sge entries to make space for the count, and given I'm not really familiar with the driver or HW, I'd be concerned that this could cause a performance regression. Overall, my preference is to leave it as is. That said, while investigating this, I've spotted a bug in my change. paddr calculation in sil24_qc_issue() is incorrect since sizeof(*pp->cmd_block) is no longer PAGE_SIZE. Based on feedback in another patch, I'm also converting the BUG_ONs to WARN_ON_ONCEs. Additional proposed change, which I'll plan to include in the next version: ---8<--- diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c index 85c6382976626..c402bf998c4ee 100644 --- a/drivers/ata/sata_sil24.c +++ b/drivers/ata/sata_sil24.c @@ -257,6 +257,10 @@ union sil24_cmd_block { struct sil24_atapi_block atapi; }; +#define SIL24_ATA_BLOCK_SIZE struct_size_t(struct sil24_ata_block, sge, SIL24_MAX_SGE) +#define SIL24_ATAPI_BLOCK_SIZE struct_size_t(struct sil24_atapi_block, sge, SIL24_MAX_SGE) +#define SIL24_CMD_BLOCK_SIZE max(SIL24_ATA_BLOCK_SIZE, SIL24_ATAPI_BLOCK_SIZE) + static const struct sil24_cerr_info { unsigned int err_mask, action; const char *desc; @@ -886,7 +890,7 @@ static unsigned int sil24_qc_issue(struct ata_queued_cmd *qc) dma_addr_t paddr; void __iomem *activate; - paddr = pp->cmd_block_dma + tag * sizeof(*pp->cmd_block); + paddr = pp->cmd_block_dma + tag * SIL24_CMD_BLOCK_SIZE; activate = port + PORT_CMD_ACTIVATE + tag * 8; /* @@ -1192,7 +1196,7 @@ static int sil24_port_start(struct ata_port *ap) struct device *dev = ap->host->dev; struct sil24_port_priv *pp; union sil24_cmd_block *cb; - size_t cb_size = PAGE_SIZE * SIL24_MAX_CMDS; + size_t cb_size = SIL24_CMD_BLOCK_SIZE * SIL24_MAX_CMDS; dma_addr_t cb_dma; pp = devm_kzalloc(dev, sizeof(*pp), GFP_KERNEL); @@ -1265,8 +1269,8 @@ static int sil24_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) u32 tmp; /* union sil24_cmd_block must be PAGE_SIZE */ - BUG_ON(struct_size_t(struct sil24_atapi_block, sge, SIL24_MAX_SGE) != PAGE_SIZE); - BUG_ON(struct_size_t(struct sil24_ata_block, sge, SIL24_MAX_SGE) > PAGE_SIZE); + WARN_ON_ONCE(SIL24_ATAPI_BLOCK_SIZE != PAGE_SIZE); + WARN_ON_ONCE(SIL24_ATA_BLOCK_SIZE != PAGE_SIZE - 16); ata_print_version_once(&pdev->dev, DRV_VERSION); ---8<--- > > >> >>> >>> I haven't looked at the DEFINE_GLOBAL_PAGE_SIZE_VAR_CONST macro, but since >> >> DEFINE_GLOBAL_PAGE_SIZE_VAR_CONST(), when doing a boot-time page size build, >> defers the initialization of the global variable to kernel init time, when >> PAGE_SIZE is known. Because SIL24_MAX_SGE is defined in terms of PAGE_SIZE, this >> deferral is required. >> >>> sge[] now becomes a flexible array member, I think it would be nice if it >>> would be possible to somehow use the __counted_by macro. >>>
On Mon, Oct 21, 2024 at 10:24:37AM +0100, Ryan Roberts wrote: > On 17/10/2024 13:51, Niklas Cassel wrote: > > On Thu, Oct 17, 2024 at 01:42:22PM +0100, Ryan Roberts wrote: (snip) > That said, while investigating this, I've spotted a bug in my change. paddr calculation in sil24_qc_issue() is incorrect since sizeof(*pp->cmd_block) is no longer PAGE_SIZE. Based on feedback in another patch, I'm also converting the BUG_ONs to WARN_ON_ONCEs. Side note: Please wrap you lines to 80 characters max. > > Additional proposed change, which I'll plan to include in the next version: > > ---8<--- > diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c > index 85c6382976626..c402bf998c4ee 100644 > --- a/drivers/ata/sata_sil24.c > +++ b/drivers/ata/sata_sil24.c > @@ -257,6 +257,10 @@ union sil24_cmd_block { > struct sil24_atapi_block atapi; > }; > > +#define SIL24_ATA_BLOCK_SIZE struct_size_t(struct sil24_ata_block, sge, SIL24_MAX_SGE) > +#define SIL24_ATAPI_BLOCK_SIZE struct_size_t(struct sil24_atapi_block, sge, SIL24_MAX_SGE) > +#define SIL24_CMD_BLOCK_SIZE max(SIL24_ATA_BLOCK_SIZE, SIL24_ATAPI_BLOCK_SIZE) > + > static const struct sil24_cerr_info { > unsigned int err_mask, action; > const char *desc; > @@ -886,7 +890,7 @@ static unsigned int sil24_qc_issue(struct ata_queued_cmd *qc) > dma_addr_t paddr; > void __iomem *activate; > > - paddr = pp->cmd_block_dma + tag * sizeof(*pp->cmd_block); > + paddr = pp->cmd_block_dma + tag * SIL24_CMD_BLOCK_SIZE; > activate = port + PORT_CMD_ACTIVATE + tag * 8; > > /* > @@ -1192,7 +1196,7 @@ static int sil24_port_start(struct ata_port *ap) > struct device *dev = ap->host->dev; > struct sil24_port_priv *pp; > union sil24_cmd_block *cb; > - size_t cb_size = PAGE_SIZE * SIL24_MAX_CMDS; > + size_t cb_size = SIL24_CMD_BLOCK_SIZE * SIL24_MAX_CMDS; > dma_addr_t cb_dma; > > pp = devm_kzalloc(dev, sizeof(*pp), GFP_KERNEL); > @@ -1265,8 +1269,8 @@ static int sil24_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > u32 tmp; > > /* union sil24_cmd_block must be PAGE_SIZE */ This comment should probably be rephrased to be more clear then, since like you said sizeof(union sil24_cmd_block) will no longer be PAGE_SIZE. > - BUG_ON(struct_size_t(struct sil24_atapi_block, sge, SIL24_MAX_SGE) != PAGE_SIZE); > - BUG_ON(struct_size_t(struct sil24_ata_block, sge, SIL24_MAX_SGE) > PAGE_SIZE); > + WARN_ON_ONCE(SIL24_ATAPI_BLOCK_SIZE != PAGE_SIZE); > + WARN_ON_ONCE(SIL24_ATA_BLOCK_SIZE != PAGE_SIZE - 16); > > ata_print_version_once(&pdev->dev, DRV_VERSION); > ---8<---
On 21/10/2024 12:04, Niklas Cassel wrote: > On Mon, Oct 21, 2024 at 10:24:37AM +0100, Ryan Roberts wrote: >> On 17/10/2024 13:51, Niklas Cassel wrote: >>> On Thu, Oct 17, 2024 at 01:42:22PM +0100, Ryan Roberts wrote: > > (snip) > >> That said, while investigating this, I've spotted a bug in my change. paddr calculation in sil24_qc_issue() is incorrect since sizeof(*pp->cmd_block) is no longer PAGE_SIZE. Based on feedback in another patch, I'm also converting the BUG_ONs to WARN_ON_ONCEs. > > Side note: Please wrap you lines to 80 characters max. Yes sorry, I turned off line wrapping for that last mail because I didn't want it to wrap the copy/pasted patch. I'll figure out how to mix and match for future. > > >> >> Additional proposed change, which I'll plan to include in the next version: >> >> ---8<--- >> diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c >> index 85c6382976626..c402bf998c4ee 100644 >> --- a/drivers/ata/sata_sil24.c >> +++ b/drivers/ata/sata_sil24.c >> @@ -257,6 +257,10 @@ union sil24_cmd_block { >> struct sil24_atapi_block atapi; >> }; >> >> +#define SIL24_ATA_BLOCK_SIZE struct_size_t(struct sil24_ata_block, sge, SIL24_MAX_SGE) >> +#define SIL24_ATAPI_BLOCK_SIZE struct_size_t(struct sil24_atapi_block, sge, SIL24_MAX_SGE) >> +#define SIL24_CMD_BLOCK_SIZE max(SIL24_ATA_BLOCK_SIZE, SIL24_ATAPI_BLOCK_SIZE) >> + >> static const struct sil24_cerr_info { >> unsigned int err_mask, action; >> const char *desc; >> @@ -886,7 +890,7 @@ static unsigned int sil24_qc_issue(struct ata_queued_cmd *qc) >> dma_addr_t paddr; >> void __iomem *activate; >> >> - paddr = pp->cmd_block_dma + tag * sizeof(*pp->cmd_block); >> + paddr = pp->cmd_block_dma + tag * SIL24_CMD_BLOCK_SIZE; >> activate = port + PORT_CMD_ACTIVATE + tag * 8; >> >> /* >> @@ -1192,7 +1196,7 @@ static int sil24_port_start(struct ata_port *ap) >> struct device *dev = ap->host->dev; >> struct sil24_port_priv *pp; >> union sil24_cmd_block *cb; >> - size_t cb_size = PAGE_SIZE * SIL24_MAX_CMDS; >> + size_t cb_size = SIL24_CMD_BLOCK_SIZE * SIL24_MAX_CMDS; >> dma_addr_t cb_dma; >> >> pp = devm_kzalloc(dev, sizeof(*pp), GFP_KERNEL); >> @@ -1265,8 +1269,8 @@ static int sil24_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) >> u32 tmp; >> >> /* union sil24_cmd_block must be PAGE_SIZE */ > > This comment should probably be rephrased to be more clear then, since like > you said sizeof(union sil24_cmd_block) will no longer be PAGE_SIZE. How about: /* * union sil24_cmd_block must be PAGE_SIZE once taking into account the 'sge' * flexible array members in struct sil24_atapi_block and struct sil24_ata_block */ > > >> - BUG_ON(struct_size_t(struct sil24_atapi_block, sge, SIL24_MAX_SGE) != PAGE_SIZE); >> - BUG_ON(struct_size_t(struct sil24_ata_block, sge, SIL24_MAX_SGE) > PAGE_SIZE); >> + WARN_ON_ONCE(SIL24_ATAPI_BLOCK_SIZE != PAGE_SIZE); >> + WARN_ON_ONCE(SIL24_ATA_BLOCK_SIZE != PAGE_SIZE - 16); >> >> ata_print_version_once(&pdev->dev, DRV_VERSION); >> ---8<---
On Mon, Oct 21, 2024 at 12:26:15PM +0100, Ryan Roberts wrote: > On 21/10/2024 12:04, Niklas Cassel wrote: > > On Mon, Oct 21, 2024 at 10:24:37AM +0100, Ryan Roberts wrote: > >> On 17/10/2024 13:51, Niklas Cassel wrote: > >>> On Thu, Oct 17, 2024 at 01:42:22PM +0100, Ryan Roberts wrote: > > > > (snip) > > > >> That said, while investigating this, I've spotted a bug in my change. paddr calculation in sil24_qc_issue() is incorrect since sizeof(*pp->cmd_block) is no longer PAGE_SIZE. Based on feedback in another patch, I'm also converting the BUG_ONs to WARN_ON_ONCEs. > > > > Side note: Please wrap you lines to 80 characters max. > > Yes sorry, I turned off line wrapping for that last mail because I didn't want > it to wrap the copy/pasted patch. I'll figure out how to mix and match for future. > > > > > > >> > >> Additional proposed change, which I'll plan to include in the next version: > >> > >> ---8<--- > >> diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c > >> index 85c6382976626..c402bf998c4ee 100644 > >> --- a/drivers/ata/sata_sil24.c > >> +++ b/drivers/ata/sata_sil24.c > >> @@ -257,6 +257,10 @@ union sil24_cmd_block { > >> struct sil24_atapi_block atapi; > >> }; > >> > >> +#define SIL24_ATA_BLOCK_SIZE struct_size_t(struct sil24_ata_block, sge, SIL24_MAX_SGE) > >> +#define SIL24_ATAPI_BLOCK_SIZE struct_size_t(struct sil24_atapi_block, sge, SIL24_MAX_SGE) > >> +#define SIL24_CMD_BLOCK_SIZE max(SIL24_ATA_BLOCK_SIZE, SIL24_ATAPI_BLOCK_SIZE) > >> + > >> static const struct sil24_cerr_info { > >> unsigned int err_mask, action; > >> const char *desc; > >> @@ -886,7 +890,7 @@ static unsigned int sil24_qc_issue(struct ata_queued_cmd *qc) > >> dma_addr_t paddr; > >> void __iomem *activate; > >> > >> - paddr = pp->cmd_block_dma + tag * sizeof(*pp->cmd_block); > >> + paddr = pp->cmd_block_dma + tag * SIL24_CMD_BLOCK_SIZE; > >> activate = port + PORT_CMD_ACTIVATE + tag * 8; > >> > >> /* > >> @@ -1192,7 +1196,7 @@ static int sil24_port_start(struct ata_port *ap) > >> struct device *dev = ap->host->dev; > >> struct sil24_port_priv *pp; > >> union sil24_cmd_block *cb; > >> - size_t cb_size = PAGE_SIZE * SIL24_MAX_CMDS; > >> + size_t cb_size = SIL24_CMD_BLOCK_SIZE * SIL24_MAX_CMDS; > >> dma_addr_t cb_dma; > >> > >> pp = devm_kzalloc(dev, sizeof(*pp), GFP_KERNEL); > >> @@ -1265,8 +1269,8 @@ static int sil24_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > >> u32 tmp; > >> > >> /* union sil24_cmd_block must be PAGE_SIZE */ > > > > This comment should probably be rephrased to be more clear then, since like > > you said sizeof(union sil24_cmd_block) will no longer be PAGE_SIZE. > > How about: > > /* > * union sil24_cmd_block must be PAGE_SIZE once taking into account the 'sge' > * flexible array members in struct sil24_atapi_block and struct sil24_ata_block > */ Sounds good to me! Kind regards, Niklas
diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c index 72c03cbdaff43..85c6382976626 100644 --- a/drivers/ata/sata_sil24.c +++ b/drivers/ata/sata_sil24.c @@ -42,26 +42,25 @@ struct sil24_sge { __le32 flags; }; +/* + * sil24 fetches in chunks of 64bytes. The first block + * contains the PRB and two SGEs. From the second block, it's + * consisted of four SGEs and called SGT. Calculate the + * number of SGTs that fit into one page. + */ +#define SIL24_PRB_SZ (sizeof(struct sil24_prb) + 2 * sizeof(struct sil24_sge)) +#define SIL24_MAX_SGT ((PAGE_SIZE - SIL24_PRB_SZ) / (4 * sizeof(struct sil24_sge))) + +/* + * This will give us one unused SGEs for ATA. This extra SGE + * will be used to store CDB for ATAPI devices. + */ +#define SIL24_MAX_SGE (4 * SIL24_MAX_SGT + 1) enum { SIL24_HOST_BAR = 0, SIL24_PORT_BAR = 2, - /* sil24 fetches in chunks of 64bytes. The first block - * contains the PRB and two SGEs. From the second block, it's - * consisted of four SGEs and called SGT. Calculate the - * number of SGTs that fit into one page. - */ - SIL24_PRB_SZ = sizeof(struct sil24_prb) - + 2 * sizeof(struct sil24_sge), - SIL24_MAX_SGT = (PAGE_SIZE - SIL24_PRB_SZ) - / (4 * sizeof(struct sil24_sge)), - - /* This will give us one unused SGEs for ATA. This extra SGE - * will be used to store CDB for ATAPI devices. - */ - SIL24_MAX_SGE = 4 * SIL24_MAX_SGT + 1, - /* * Global controller registers (128 bytes @ BAR0) */ @@ -244,13 +243,13 @@ enum { struct sil24_ata_block { struct sil24_prb prb; - struct sil24_sge sge[SIL24_MAX_SGE]; + struct sil24_sge sge[]; }; struct sil24_atapi_block { struct sil24_prb prb; u8 cdb[16]; - struct sil24_sge sge[SIL24_MAX_SGE]; + struct sil24_sge sge[]; }; union sil24_cmd_block { @@ -373,7 +372,7 @@ static struct pci_driver sil24_pci_driver = { #endif }; -static const struct scsi_host_template sil24_sht = { +static DEFINE_GLOBAL_PAGE_SIZE_VAR_CONST(struct scsi_host_template, sil24_sht, { __ATA_BASE_SHT(DRV_NAME), .can_queue = SIL24_MAX_CMDS, .sg_tablesize = SIL24_MAX_SGE, @@ -382,7 +381,7 @@ static const struct scsi_host_template sil24_sht = { .sdev_groups = ata_ncq_sdev_groups, .change_queue_depth = ata_scsi_change_queue_depth, .device_configure = ata_scsi_device_configure -}; +}); static struct ata_port_operations sil24_ops = { .inherits = &sata_pmp_port_ops, @@ -1193,7 +1192,7 @@ static int sil24_port_start(struct ata_port *ap) struct device *dev = ap->host->dev; struct sil24_port_priv *pp; union sil24_cmd_block *cb; - size_t cb_size = sizeof(*cb) * SIL24_MAX_CMDS; + size_t cb_size = PAGE_SIZE * SIL24_MAX_CMDS; dma_addr_t cb_dma; pp = devm_kzalloc(dev, sizeof(*pp), GFP_KERNEL); @@ -1258,7 +1257,6 @@ static void sil24_init_controller(struct ata_host *host) static int sil24_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) { - extern int __MARKER__sil24_cmd_block_is_sized_wrongly; struct ata_port_info pi = sil24_port_info[ent->driver_data]; const struct ata_port_info *ppi[] = { &pi, NULL }; void __iomem * const *iomap; @@ -1266,9 +1264,9 @@ static int sil24_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) int rc; u32 tmp; - /* cause link error if sil24_cmd_block is sized wrongly */ - if (sizeof(union sil24_cmd_block) != PAGE_SIZE) - __MARKER__sil24_cmd_block_is_sized_wrongly = 1; + /* union sil24_cmd_block must be PAGE_SIZE */ + BUG_ON(struct_size_t(struct sil24_atapi_block, sge, SIL24_MAX_SGE) != PAGE_SIZE); + BUG_ON(struct_size_t(struct sil24_ata_block, sge, SIL24_MAX_SGE) > PAGE_SIZE); ata_print_version_once(&pdev->dev, DRV_VERSION);
To prepare for supporting boot-time page size selection, refactor code to remove assumptions about PAGE_SIZE being compile-time constant. Code intended to be equivalent when compile-time page size is active. Convert "struct sil24_ata_block" and "struct sil24_atapi_block" to use a flexible array member for their sge[] array. The previous static size of SIL24_MAX_SGE depends on PAGE_SIZE so doesn't work for boot-time page size. Wrap global variables that are initialized with PAGE_SIZE derived values using DEFINE_GLOBAL_PAGE_SIZE_VAR() so their initialization can be deferred for boot-time page size builds. Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> --- ***NOTE*** Any confused maintainers may want to read the cover note here for context: https://lore.kernel.org/all/20241014105514.3206191-1-ryan.roberts@arm.com/ drivers/ata/sata_sil24.c | 46 +++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 24 deletions(-)