Re: [PATCH v3 2/3] PCI/NPEM: Add Native PCIe Enclosure Management support

[Lukas Wunner <lukas@wunner.de>]

On Mon, Jul 08, 2024 at 02:33:34PM +0300, Ilpo Järvinen wrote:
> > +static int npem_set_active_indications(struct npem *npem, u32 inds)
> > +{
> > +	int ctrl, ret, ret_val;
> > +	u32 cc_status;
> > +
> > +	lockdep_assert_held(&npem->lock);
> > +
> > +	/* This bit is always required */
> > +	ctrl = inds | PCI_NPEM_CTRL_ENABLE;
> > +
> > +	ret = npem_write_ctrl(npem, ctrl);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * For the case where a NPEM command has not completed immediately,
> > +	 * it is recommended that software not continuously ???spin??? on polling
> > +	 * the status register, but rather poll under interrupt at a reduced
> > +	 * rate; for example at 10 ms intervals.
> > +	 *
> > +	 * PCIe r6.1 sec 6.28 "Implementation Note: Software Polling of NPEM
> > +	 * Command Completed"
> > +	 */
> > +	ret = read_poll_timeout(npem_read_reg, ret_val,
> > +				ret_val || (cc_status & PCI_NPEM_STATUS_CC),
> > +				10 * USEC_PER_MSEC, USEC_PER_SEC, false, npem,
> > +				PCI_NPEM_STATUS, &cc_status);
> > +	if (ret)
> > +		return ret_val;
> 
> Will this work as intended?
> 
> If ret_val gets set, cond in read_poll_timeout() is true and it returns 0 
> so the return branch is not taken.
> 
> Also, when read_poll_timeout() times out, ret_val might not be non-zero.

Hm, it seems to me that just having two if-clauses should fix that.

+	ret = read_poll_timeout(npem_read_reg, ret_val,
+				ret_val || (cc_status & PCI_NPEM_STATUS_CC),
+				10 * USEC_PER_MSEC, USEC_PER_SEC, false, npem,
+				PCI_NPEM_STATUS, &cc_status);
+	if (ret)
+		return ret;
+	if (ret_val)
+		return ret_val;

Does this look correct to you?

Thanks,

Lukas