| Message ID | 20220121135718.27172-1-pali@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Support for PROGIF, REVID and SUBSYS | expand |
Hello! > libpci currently provides only access to bits [23:8] of class id via > dev->device_class member. Remaining bits [7:0] of class id can be only > accessed via reading config space. [...] I really do not like the explosion of PCI_FILL_xxx flags for trivial things. Add a single PCI_FILL_CLASS_EXT, which will fill the class, subclass, revision and programming interface. Martin
On Friday 21 January 2022 15:40:14 Martin Mareš wrote: > Hello! > > > libpci currently provides only access to bits [23:8] of class id via > > dev->device_class member. Remaining bits [7:0] of class id can be only > > accessed via reading config space. > [...] > > I really do not like the explosion of PCI_FILL_xxx flags for trivial things. > > Add a single PCI_FILL_CLASS_EXT, which will fill the class, subclass, > revision and programming interface. Ok! How to handle situation when "class+subclass+prog_if" is provided and revision is not provided? What should libpci backends set in this case? Because on both Linux and Windows systems are these information provided separately. On Linux you can chmod 000 revision sysfs file and let class sysfs file still readable. Windows can probably decide itself that it would not report revision at all... And what to do with subsystem ids? They are not part of class/subclass/prog_if/revision fields and different devices have them stored on different locations... And for PCI-to-PCI bridges they are only optional and does not have to be present at all.
Hello! > How to handle situation when "class+subclass+prog_if" is provided and > revision is not provided? What should libpci backends set in this case? > Because on both Linux and Windows systems are these information provided > separately. On Linux you can chmod 000 revision sysfs file and let class > sysfs file still readable. Windows can probably decide itself that it > would not report revision at all... Read it from the config registers in that case. > And what to do with subsystem ids? They are not part of > class/subclass/prog_if/revision fields and different devices have them > stored on different locations... And for PCI-to-PCI bridges they are > only optional and does not have to be present at all. I would prefer a separate PCI_FILL_xxx flag for them. Have a nice fortnight
On Friday 21 January 2022 16:15:32 Martin Mareš wrote: > Hello! > > > How to handle situation when "class+subclass+prog_if" is provided and > > revision is not provided? What should libpci backends set in this case? > > Because on both Linux and Windows systems are these information provided > > separately. On Linux you can chmod 000 revision sysfs file and let class > > sysfs file still readable. Windows can probably decide itself that it > > would not report revision at all... > > Read it from the config registers in that case. On Linux "class+subclass+prog_if" can be different than what is in config registers and the purpose of this patch series with new fields is to allow user to see difference in lspci. On Windows access to real config space is not available for non-system users. > > And what to do with subsystem ids? They are not part of > > class/subclass/prog_if/revision fields and different devices have them > > stored on different locations... And for PCI-to-PCI bridges they are > > only optional and does not have to be present at all. > > I would prefer a separate PCI_FILL_xxx flag for them. So like PCI_FILL_SUBSYS flag in this patch series?
> On Linux "class+subclass+prog_if" can be different than what is in > config registers and the purpose of this patch series with new fields is > to allow user to see difference in lspci. > > On Windows access to real config space is not available for non-system > users. Sure, I meant to read it from the config registers only if the back-end is unable to supply it. > > > And what to do with subsystem ids? They are not part of > > > class/subclass/prog_if/revision fields and different devices have them > > > stored on different locations... And for PCI-to-PCI bridges they are > > > only optional and does not have to be present at all. > > > > I would prefer a separate PCI_FILL_xxx flag for them. > > So like PCI_FILL_SUBSYS flag in this patch series? Yes, that part was OK. Have a nice fortnight
On Friday 21 January 2022 16:29:44 Martin Mareš wrote: > > On Linux "class+subclass+prog_if" can be different than what is in > > config registers and the purpose of this patch series with new fields is > > to allow user to see difference in lspci. > > > > On Windows access to real config space is not available for non-system > > users. > > Sure, I meant to read it from the config registers only if the back-end > is unable to supply it. And in case "class+subclass+prog_if" is available but "revision" is not available and even it is not possible to read revision from config space (because e.g. due to missing permissions or limitations of win backend)?
> And in case "class+subclass+prog_if" is available but "revision" is not > available and even it is not possible to read revision from config space > (because e.g. due to missing permissions or limitations of win backend)? I bet that nobody will ever check if reading of something as basic as the revision ID succeeded. It can never fail on sane systems. In such cases, just return a default value and log a warning. Have a nice fortnight
On Friday 21 January 2022 16:38:50 Martin Mareš wrote: > > And in case "class+subclass+prog_if" is available but "revision" is not > > available and even it is not possible to read revision from config space > > (because e.g. due to missing permissions or limitations of win backend)? > > I bet that nobody will ever check if reading of something as basic as the > revision ID succeeded. It can never fail on sane systems. > > In such cases, just return a default value and log a warning. Ok! Sorry for a longer discussion as it is only corner case, but it can happen (probably more often happens on windows) and therefore needs to be somehow handled in the code.