diff mbox series

PCI/VPD: Silence warning if optional VPD PROM is missing

Message ID b04a0e46-0b97-da3d-aa77-b05c9b37d21f@gmail.com (mailing list archive)
State New
Delegated to: Bjorn Helgaas
Headers show
Series PCI/VPD: Silence warning if optional VPD PROM is missing | expand

Commit Message

Heiner Kallweit Dec. 17, 2020, 8:43 p.m. UTC
Realtek RTL8169/8168/8125 NIC families indicate VPD capability and an
optional VPD EEPROM can be connected via I2C/SPI. However I haven't
seen any card or system with such a VPD EEPROM yet. The missing EEPROM
causes the following warning whenever e.g. lscpi -vv is executed.

invalid short VPD tag 00 at offset 01

The warning confuses users, I think we should handle the situation more
gentle. Therefore, if first VPD byte is read as 0x00, assume a missing
optional VPD PROM as and silently set the VPD length to 0.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/pci/vpd.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Krzysztof Wilczyński March 7, 2021, 6:27 p.m. UTC | #1
Hi Heiner,

> Realtek RTL8169/8168/8125 NIC families indicate VPD capability and an
> optional VPD EEPROM can be connected via I2C/SPI. However I haven't
> seen any card or system with such a VPD EEPROM yet. The missing EEPROM
> causes the following warning whenever e.g. lscpi -vv is executed.
> 
> invalid short VPD tag 00 at offset 01
> 
> The warning confuses users, I think we should handle the situation more
> gentle. Therefore, if first VPD byte is read as 0x00, assume a missing
> optional VPD PROM as and silently set the VPD length to 0.
[...]

True.  I saw people on different forum and IRC asking for clarification
assuming their NIC broke, or that something is wrong, so this would
indeed save them some worry, nice!

Having said that, I also saw this particular warning showing up for some
storage controllers (often some SAS cards), so a question here: would it
warrant adding a pci_dbg() with an appropriate message rather than just
returning 0?  I wonder if this might be useful for someone who is trying
to troubleshoot and/or debug some issues with their device.

What do you think?

Krzysztof
Heiner Kallweit March 7, 2021, 9:34 p.m. UTC | #2
On 07.03.2021 19:27, Krzysztof Wilczyński wrote:
> Hi Heiner,
> 
>> Realtek RTL8169/8168/8125 NIC families indicate VPD capability and an
>> optional VPD EEPROM can be connected via I2C/SPI. However I haven't
>> seen any card or system with such a VPD EEPROM yet. The missing EEPROM
>> causes the following warning whenever e.g. lscpi -vv is executed.
>>
>> invalid short VPD tag 00 at offset 01
>>
>> The warning confuses users, I think we should handle the situation more
>> gentle. Therefore, if first VPD byte is read as 0x00, assume a missing
>> optional VPD PROM as and silently set the VPD length to 0.
> [...]
> 
> True.  I saw people on different forum and IRC asking for clarification
> assuming their NIC broke, or that something is wrong, so this would
> indeed save them some worry, nice!
> 
> Having said that, I also saw this particular warning showing up for some
> storage controllers (often some SAS cards), so a question here: would it
> warrant adding a pci_dbg() with an appropriate message rather than just
> returning 0?  I wonder if this might be useful for someone who is trying
> to troubleshoot and/or debug some issues with their device.
> 
> What do you think?
> 
I don't have a strong opinion here, but yes, that's something we could do.

> Krzysztof
>
Bjorn Helgaas March 30, 2021, 7:56 p.m. UTC | #3
On Sun, Mar 07, 2021 at 10:34:25PM +0100, Heiner Kallweit wrote:
> On 07.03.2021 19:27, Krzysztof Wilczyński wrote:
> > Hi Heiner,
> > 
> >> Realtek RTL8169/8168/8125 NIC families indicate VPD capability and an
> >> optional VPD EEPROM can be connected via I2C/SPI. However I haven't
> >> seen any card or system with such a VPD EEPROM yet. The missing EEPROM
> >> causes the following warning whenever e.g. lscpi -vv is executed.
> >>
> >> invalid short VPD tag 00 at offset 01
> >>
> >> The warning confuses users, I think we should handle the situation more
> >> gentle. Therefore, if first VPD byte is read as 0x00, assume a missing
> >> optional VPD PROM as and silently set the VPD length to 0.
> > [...]
> > 
> > True.  I saw people on different forum and IRC asking for clarification
> > assuming their NIC broke, or that something is wrong, so this would
> > indeed save them some worry, nice!
> > 
> > Having said that, I also saw this particular warning showing up for some
> > storage controllers (often some SAS cards), so a question here: would it
> > warrant adding a pci_dbg() with an appropriate message rather than just
> > returning 0?  I wonder if this might be useful for someone who is trying
> > to troubleshoot and/or debug some issues with their device.
> > 
> > What do you think?
> > 
> I don't have a strong opinion here, but yes, that's something we could do.

How about if we just downgrade the pci_warn() to a pci_info()?
Heiner Kallweit March 31, 2021, 11:14 a.m. UTC | #4
On 30.03.2021 21:56, Bjorn Helgaas wrote:
> On Sun, Mar 07, 2021 at 10:34:25PM +0100, Heiner Kallweit wrote:
>> On 07.03.2021 19:27, Krzysztof Wilczyński wrote:
>>> Hi Heiner,
>>>
>>>> Realtek RTL8169/8168/8125 NIC families indicate VPD capability and an
>>>> optional VPD EEPROM can be connected via I2C/SPI. However I haven't
>>>> seen any card or system with such a VPD EEPROM yet. The missing EEPROM
>>>> causes the following warning whenever e.g. lscpi -vv is executed.
>>>>
>>>> invalid short VPD tag 00 at offset 01
>>>>
>>>> The warning confuses users, I think we should handle the situation more
>>>> gentle. Therefore, if first VPD byte is read as 0x00, assume a missing
>>>> optional VPD PROM as and silently set the VPD length to 0.
>>> [...]
>>>
>>> True.  I saw people on different forum and IRC asking for clarification
>>> assuming their NIC broke, or that something is wrong, so this would
>>> indeed save them some worry, nice!
>>>
>>> Having said that, I also saw this particular warning showing up for some
>>> storage controllers (often some SAS cards), so a question here: would it
>>> warrant adding a pci_dbg() with an appropriate message rather than just
>>> returning 0?  I wonder if this might be useful for someone who is trying
>>> to troubleshoot and/or debug some issues with their device.
>>>
>>> What do you think?
>>>
>> I don't have a strong opinion here, but yes, that's something we could do.
> 
> How about if we just downgrade the pci_warn() to a pci_info()?
> 
pci_info() would still expose a quite cryptic message to users and leave
them with the question whether something is wrong. If in case of VPD tag 00
a message is desired, I'd say it should be rephrased to something like:
"VPD tag 00 at offset 01, assuming missing optional VPD EPROM"
Heiner Kallweit April 1, 2021, 12:04 p.m. UTC | #5
On 31.03.2021 13:14, Heiner Kallweit wrote:
> On 30.03.2021 21:56, Bjorn Helgaas wrote:
>> On Sun, Mar 07, 2021 at 10:34:25PM +0100, Heiner Kallweit wrote:
>>> On 07.03.2021 19:27, Krzysztof Wilczyński wrote:
>>>> Hi Heiner,
>>>>
>>>>> Realtek RTL8169/8168/8125 NIC families indicate VPD capability and an
>>>>> optional VPD EEPROM can be connected via I2C/SPI. However I haven't
>>>>> seen any card or system with such a VPD EEPROM yet. The missing EEPROM
>>>>> causes the following warning whenever e.g. lscpi -vv is executed.
>>>>>
>>>>> invalid short VPD tag 00 at offset 01
>>>>>
>>>>> The warning confuses users, I think we should handle the situation more
>>>>> gentle. Therefore, if first VPD byte is read as 0x00, assume a missing
>>>>> optional VPD PROM as and silently set the VPD length to 0.
>>>> [...]
>>>>
>>>> True.  I saw people on different forum and IRC asking for clarification
>>>> assuming their NIC broke, or that something is wrong, so this would
>>>> indeed save them some worry, nice!
>>>>
>>>> Having said that, I also saw this particular warning showing up for some
>>>> storage controllers (often some SAS cards), so a question here: would it
>>>> warrant adding a pci_dbg() with an appropriate message rather than just
>>>> returning 0?  I wonder if this might be useful for someone who is trying
>>>> to troubleshoot and/or debug some issues with their device.
>>>>
>>>> What do you think?
>>>>
>>> I don't have a strong opinion here, but yes, that's something we could do.
>>
>> How about if we just downgrade the pci_warn() to a pci_info()?
>>
> pci_info() would still expose a quite cryptic message to users and leave
> them with the question whether something is wrong. If in case of VPD tag 00
> a message is desired, I'd say it should be rephrased to something like:
> "VPD tag 00 at offset 01, assuming missing optional VPD EPROM"
> 

I submitted a v2 with this change.
diff mbox series

Patch

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index ef5165eb3..bd174705f 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -89,6 +89,10 @@  static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
 	       pci_read_vpd(dev, off, 1, header) == 1) {
 		unsigned char tag;
 
+		/* assume missing optional VPD PROM */
+		if (!header[0] && !off)
+			return 0;
+
 		if (header[0] & PCI_VPD_LRDT) {
 			/* Large Resource Data Type Tag */
 			tag = pci_vpd_lrdt_tag(header);