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

[Xuan Zhuo <xuanzhuo@linux.alibaba.com>]

On Thu, 2 Apr 2026 10:02:53 +0200, Paolo Abeni <pabeni@redhat.com> wrote:
> 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.


Yes, we had indeed overlooked this part before. I think it's still better to
keep it 64-bit aligned. AI did help us, and additionally, it caught another
issue: if an RX packet is dropped, we need to re-dma-sync the buffer
for the device when reusing it. In this AI review, I believe only these two
findings are valid. Even though the second one rarely causes problems in
practice, I'll submit another patch set to address both. AI does catch real
issues, but the Sashiko review results seem to generate too many false
positives. Reviewing them takes up too much of our time, and the return on
effort is too low. I've gone through every single item flagged by Sashiko, and
only these two are actual issues; the rest don't exist. Unfortunately, I can't
reply to them inline like I would in an email thread.

>
> [ ... ]
> > +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.


I will add check for 0xFF.


Thanks


>
> > +
> > +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
>