Re: [PATCH 1/5] PCI: add resizeable BAR infrastructure v2

From: Andy Shevchenko
Date: Mon Mar 06 2017 - 07:20:49 EST


On Mon, Mar 6, 2017 at 1:40 PM, Christian KÃnig <deathsimple@xxxxxxxxxxx> wrote:
> From: Christian KÃnig <christian.koenig@xxxxxxx>
>
> Just the defines and helper functions to read the possible sizes of a BAR and
> update it's size.
>
> See https://pcisig.com/sites/default/files/specification_documents/ECN_Resizable-BAR_24Apr2008.pdf.
>
> v2: provide read helper as well

Commit message left away the explanation at which point this API might
be useful and how it fits in managed resources model?

> /**
> + * pci_rbar_get_sizes - get possible sizes for BAR

Why not simple pci_rbar_get_possible_sizes() ?

> +u32 pci_rbar_get_sizes(struct pci_dev *pdev, int bar)
> +{
> + int pos, nbars;
> + u32 ctrl, cap;
> + int i;
> +
> + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_REBAR);
> + if (!pos)

> + return 0x0;

return 0;

> +
> + pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> + nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >> PCI_REBAR_CTRL_NBAR_SHIFT;
> +

> + for (i = 0; i < nbars; ++i, pos += 8) {

8 is defined somewhere in the spec? (Yes, I understand that is just 64
bits shift)

> + int bar_idx;
> +
> + pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> + bar_idx = (ctrl & PCI_REBAR_CTRL_BAR_IDX_MASK) >>
> + PCI_REBAR_CTRL_BAR_IDX_SHIFT;
> + if (bar_idx != bar)
> + continue;
> +
> + pci_read_config_dword(pdev, pos + PCI_REBAR_CAP, &cap);
> + return (cap & PCI_REBAR_CTRL_SIZES_MASK) >>
> + PCI_REBAR_CTRL_SIZES_SHIFT;
> + }
> +

> + return 0x0;

return 0;

> +/**
> + * pci_rbar_get_size - get the current size of a BAR

pci_rbar_get_current_size() ?

> +/**
> + * pci_rbar_set_size - set a new size for a BAR
> + * @dev: PCI device
> + * @bar: BAR to set size to

> + * @size: new size as defined in the spec.

* @size: bitmasked value of new size (bit 0=1MB, ..., bit 19=512G)

?

It will briefly get a clue without reading either spec or long description.

> + *
> + * Set the new size of a BAR as defined in the spec (0=1MB, 19=512GB).
> + * Returns true if resizing was successful, false otherwise.
> + */

> +bool pci_rbar_set_size(struct pci_dev *pdev, int bar, int size)

I would return int and error code. It would be better in the future
and seems in alignment with above.

> +{
> + int pos, nbars;
> + u32 ctrl;
> + int i;

All ints are unsigned?

--
With Best Regards,
Andy Shevchenko