2024-02-26 12:10:54

by Jonathan Cameron

Subject: Re: [RFC PATCH 03/20] dev_dax_iomap: Move dax_pgoff_to_phys from device.c to bus.c since both need it now

On Fri, 23 Feb 2024 11:41:47 -0600
John Groves <[email protected]> wrote:

> bus.c can't call functions in device.c - that creates a circular linkage
> dependency.
>
> Signed-off-by: John Groves <[email protected]>

This also adds the export which you should mention!

Do they need it already? Seems like tense of patch title
may be wrong.

> ---
> drivers/dax/bus.c | 24 ++++++++++++++++++++++++
> drivers/dax/device.c | 23 -----------------------
> 2 files changed, 24 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
> index 1ff1ab5fa105..664e8c1b9930 100644
> --- a/drivers/dax/bus.c
> +++ b/drivers/dax/bus.c
> @@ -1325,6 +1325,30 @@ static const struct device_type dev_dax_type = {
> .groups = dax_attribute_groups,
> };
>
> +/* see "strong" declaration in tools/testing/nvdimm/dax-dev.c */
> +__weak phys_addr_t dax_pgoff_to_phys(struct dev_dax *dev_dax, pgoff_t pgoff,
> + unsigned long size)
> +{
> + int i;
> +
> + for (i = 0; i < dev_dax->nr_range; i++) {
> + struct dev_dax_range *dax_range = &dev_dax->ranges[i];
> + struct range *range = &dax_range->range;
> + unsigned long long pgoff_end;
> + phys_addr_t phys;
> +
> + pgoff_end = dax_range->pgoff + PHYS_PFN(range_len(range)) - 1;
> + if (pgoff < dax_range->pgoff || pgoff > pgoff_end)
> + continue;
> + phys = PFN_PHYS(pgoff - dax_range->pgoff) + range->start;
> + if (phys + size - 1 <= range->end)
> + return phys;
> + break;
> + }
> + return -1;

Not related to your patch but returning -1 in a phys_addr_t isn't ideal.
I assume aim is all bits set as a marker, in which case
PHYS_ADDR_MAX from limits.h would make things clearer.

> +}
> +EXPORT_SYMBOL_GPL(dax_pgoff_to_phys);
> +
> struct dev_dax *devm_create_dev_dax(struct dev_dax_data *data)
> {
> struct dax_region *dax_region = data->dax_region;
> diff --git a/drivers/dax/device.c b/drivers/dax/device.c
> index 93ebedc5ec8c..40ba660013cf 100644
> --- a/drivers/dax/device.c
> +++ b/drivers/dax/device.c
> @@ -50,29 +50,6 @@ static int check_vma(struct dev_dax *dev_dax, struct vm_area_struct *vma,
> return 0;
> }
>
> -/* see "strong" declaration in tools/testing/nvdimm/dax-dev.c */
> -__weak phys_addr_t dax_pgoff_to_phys(struct dev_dax *dev_dax, pgoff_t pgoff,
> - unsigned long size)
> -{
> - int i;
> -
> - for (i = 0; i < dev_dax->nr_range; i++) {
> - struct dev_dax_range *dax_range = &dev_dax->ranges[i];
> - struct range *range = &dax_range->range;
> - unsigned long long pgoff_end;
> - phys_addr_t phys;
> -
> - pgoff_end = dax_range->pgoff + PHYS_PFN(range_len(range)) - 1;
> - if (pgoff < dax_range->pgoff || pgoff > pgoff_end)
> - continue;
> - phys = PFN_PHYS(pgoff - dax_range->pgoff) + range->start;
> - if (phys + size - 1 <= range->end)
> - return phys;
> - break;
> - }
> - return -1;
> -}
> -
> static void dax_set_mapping(struct vm_fault *vmf, pfn_t pfn,
> unsigned long fault_size)
> {

2024-02-26 15:25:44

by John Groves

Subject: Re: [RFC PATCH 03/20] dev_dax_iomap: Move dax_pgoff_to_phys from device.c to bus.c since both need it now

On 24/02/26 12:10PM, Jonathan Cameron wrote:
> On Fri, 23 Feb 2024 11:41:47 -0600
> John Groves <[email protected]> wrote:
>
> > bus.c can't call functions in device.c - that creates a circular linkage
> > dependency.
> >
> > Signed-off-by: John Groves <[email protected]>
>
> This also adds the export which you should mention!
>
> Do they need it already? Seems like tense of patch title
> may be wrong.

I added "Also exports dax_pgoff_to_phys() since both bus.c and
device.c now call it."

The export is necessary because bus.c and device.c are not in the same .ko

Let me know if it seems like I'm misunderstanding...

>
> > ---
> > drivers/dax/bus.c | 24 ++++++++++++++++++++++++
> > drivers/dax/device.c | 23 -----------------------
> > 2 files changed, 24 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
> > index 1ff1ab5fa105..664e8c1b9930 100644
> > --- a/drivers/dax/bus.c
> > +++ b/drivers/dax/bus.c
> > @@ -1325,6 +1325,30 @@ static const struct device_type dev_dax_type = {
> > .groups = dax_attribute_groups,
> > };
> >
> > +/* see "strong" declaration in tools/testing/nvdimm/dax-dev.c */
> > +__weak phys_addr_t dax_pgoff_to_phys(struct dev_dax *dev_dax, pgoff_t pgoff,
> > + unsigned long size)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < dev_dax->nr_range; i++) {
> > + struct dev_dax_range *dax_range = &dev_dax->ranges[i];
> > + struct range *range = &dax_range->range;
> > + unsigned long long pgoff_end;
> > + phys_addr_t phys;
> > +
> > + pgoff_end = dax_range->pgoff + PHYS_PFN(range_len(range)) - 1;
> > + if (pgoff < dax_range->pgoff || pgoff > pgoff_end)
> > + continue;
> > + phys = PFN_PHYS(pgoff - dax_range->pgoff) + range->start;
> > + if (phys + size - 1 <= range->end)
> > + return phys;
> > + break;
> > + }
> > + return -1;
>
> Not related to your patch but returning -1 in a phys_addr_t isn't ideal.
> I assume aim is all bits set as a marker, in which case
> PHYS_ADDR_MAX from limits.h would make things clearer.

Perhaps Dan or the other dax people can comment on this? I just moved the
function verbatim, but Jonathan makes a good point!

Thanks,
John