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
