Re: [PATCH v6 04/24] PCI/sysfs: Use BAR length in pci_llseek_resource() when attr->size is zero

["Krzysztof Wilczyński" <kwilczynski@kernel.org>]

Hello,

> > @@ -909,11 +909,21 @@ static const struct attribute_group pci_dev_config_attr_group = {
> >   */
> >  static __maybe_unused loff_t
> >  pci_llseek_resource(struct file *filep,
> > -		    struct kobject *kobj __always_unused,
> > +		    struct kobject *kobj,
> >  		    const struct bin_attribute *attr,
> >  		    loff_t offset, int whence)
> >  {
> > -	return fixed_size_llseek(filep, offset, whence, attr->size);
> > +	struct pci_dev *pdev;
> > +	int bar;
> > +
> > +	if (attr->size)
> > +		return fixed_size_llseek(filep, offset, whence, attr->size);
> > +
> > +	pdev = to_pci_dev(kobj_to_dev(kobj));
> > +	bar = (unsigned long)attr->private;
> > +
> > +	return fixed_size_llseek(filep, offset, whence,
> > +				 pci_resource_len(pdev, bar));
>
> Is there a case where using "attr->size" is better than using
> "pci_resource_len(pdev, bar)"?
>
> In other words, would the following be equivalent?
>
>   pci_llseek_resource(...)
>   {
>     ...
>     pdev = to_pci_dev(kobj_to_dev(kobj));
>     bar = (unsigned long)attr->private;
>
>     return fixed_size_llseek(filep, offset, whence,
>                              pci_resource_len(pdev, bar));
>   }

Sadly, the simplified version would break legacy attributes.

pci_llseek_resource() is shared between device-level resource
attributes and bus-level legacy attributes, both have different
semantics:

  - Resource attributes (resource0, resource0_wc, ...) are per-device,
    carry a BAR index in attr->private, and will have .size == 0 with
    the static conversion.
  
  - Legacy attributes (legacy_io, legacy_mem, ...) are per-bus, have
    no BAR index in attr->private, and carry a fixed .size
    (PCI_LEGACY_IO_SIZE, PCI_LEGACY_MEM_SIZE, etc.).

The if (attr->size) check distinguishes the two cases, where legacy
attributes have size set at compile time (no BAR index), and the
resource attributes derive it from the BAR at runtime.

For legacy attributes, the kobj belongs to a struct pci_bus, not a
struct pci_dev, so to_pci_dev(kobj_to_dev(kobj)) would be a wrong
type for container_of().  Also, the pci_resource_len() helper would
not work there either.

Thus, dropping the attr->size check and always using pci_resource_len()
would break the legacy attributes case.

The alternative would be separate llseek callbacks for both the legacy
and resource attributes, which we can add if this would be the preference
here.

I hope this clears this up a little bit.

Thank you!

	Krzysztof