Re: [PATCH net-next v36 1/8] eea: introduce PCI framework

[Paolo Abeni <pabeni@redhat.com>]

On 3/29/26 3:22 PM, Xuan Zhuo wrote:
> +struct eea_pci_cfg {
> +	__le32 reserve0;
> +	__le32 reserve1;
> +	__le32 drv_f_idx;
> +	__le32 drv_f;
> +
> +#define EEA_S_OK           BIT(2)
> +#define EEA_S_FEATURE_DONE BIT(3)
> +#define EEA_S_FAILED       BIT(7)
> +	u8   device_status;
> +	u8   reserved[7];
> +
> +	__le32 rx_num_max;
> +	__le32 tx_num_max;
> +	__le32 db_blk_size;
> +
> +	/* admin queue cfg */
> +	__le16 aq_size;
> +	__le16 aq_msix_vector;
> +	__le32 aq_db_off;
> +
> +	__le32 aq_sq_addr;
> +	__le32 aq_sq_addr_hi;
> +	__le32 aq_cq_addr;
> +	__le32 aq_cq_addr_hi;
> +
> +	__le64 hw_ts;

Sashiko has still a lot to say about this series. Here:

---
Is there an implicit compiler padding issue here?
The field aq_cq_addr_hi is a 32-bit integer ending at offset 59. The
immediately following field, hw_ts, is an 8-byte integer. To align hw_ts
to an 8-byte boundary, the compiler will implicitly insert 4 bytes of
padding, placing hw_ts at offset 64 instead of 60.
Depending on whether the hardware designer intended the register to be
at offset 60 or 64, the driver might read from the wrong address, or
rely on implicit padding that could change across architectures.
---

I assume the (virtual) H/W does the align thing correctly, so no real
issue here. Still replying to this patch explaining why the raised
concern is not valid would help a lot progresses on this series.

[ ... ]
> +int eea_device_reset(struct eea_device *edev)
> +{
> +	struct eea_pci_device *ep_dev = edev->ep_dev;
> +	u8 val;
> +
> +	eea_pci_io_set_status(edev, 0);
> +
> +	return read_poll_timeout(cfg_read8, val, !val, 20, EEA_RESET_TIMEOUT_US,
> +				 false, ep_dev->reg, device_status);
> +}

Sashiko says:
---
Can this cause a 60-second thread stall during a surprise removal?
When a PCIe device is surprise-removed, MMIO reads typically return all
ones (0xFF). If the device is removed, !0xFF evaluates to false, causing
the loop to never exit early and hanging the executing thread for the
entire 60-second timeout.
---

A similar concern was raised on the previous iteration. I see you
decreased the timeout from 1000s to 60s, but 60s is still a considerably
longsystem hangup. Anything above a few seconds should be considered
carefully.

> +
> +int eea_pci_set_aq_up(struct eea_device *edev)
> +{
> +	struct eea_pci_device *ep_dev = edev->ep_dev;
> +	u8 status = eea_pci_io_get_status(edev);
> +	int err;
> +	u8 val;
> +
> +	WARN_ON(status & EEA_S_OK);

Sashiko says:

---
Does a surprise removal trigger a spurious kernel warning here?
A surprise removal forces status to 0xFF, making 0xFF & BIT(2) true and
triggering an unintended kernel stack dump for an asynchronous hardware
event.
---

This looks valid to me.

/P