Re: [PATCH v2] NTB: Add IDT 89HPESxNTx PCIe-switches support

[Serge Semin]

On Tue, Feb 21, 2017 at 05:42:37PM -0500, Allen Hubbe <Allen.Hubbe@xxxxxxxx> wrote:
> From: Serge Semin
> > +/*
> > + * idt_nt_write() - PCI configuration space registers write method
> > + * @ndev:	IDT NTB hardware driver descriptor
> > + * @reg:	Register to write data to
> > + * @data:	Value to write to the register
> > + *
> > + * WARNING! IDT PCIe-switch registers are all Little endian. So corresponding
> > + *	    writel operations must have embedded endiannes conversion. If local
> > + *	    platform doesn't have it, the driver won't properly work.
> > + */
> > +static void idt_nt_write(struct idt_ntb_dev *ndev,
> > +			 const unsigned int reg, const u32 data)
> > +{
> > +	/*
> > +	 * It's obvious bug to request a register exceeding the maximum possible
> > +	 * value as well as to have it unaligned.
> > +	 */
> > +	WARN_ON(reg > IDT_REG_PCI_MAX || !IS_ALIGNED(reg, IDT_REG_ALIGN));
> 
> If we perform the write anyway, I guess the effect of the write is unknown?
> 
> What about:
> if (WARN_ON(stuff))
> 	return;
> 

Agreed. I'll alter the registers write method.

> 
> > +/*
> > + * idt_reg_set_bits() - set bits of a passed register
> > + * @ndev:	IDT NTB hardware driver descriptor
> > + * @reg:	Register to change bits of
> > + * @valid_mask:	Mask of valid bits
> > + * @set_bits:	Bitmask to set
> > + *
> > + * Helper method to check whether a passed bitfield is valid and set
> > + * corresponding bits of a register.
> > + *
> > + * Return: zero on success, negative error on invalid bitmask.
> > + */
> > +static inline int idt_reg_set_bits(struct idt_ntb_dev *ndev, unsigned int reg,
> > +				   u64 valid_mask, u64 set_bits)
> > +{
> > +	u32 data;
> > +
> > +	if (set_bits & ~(u64)valid_mask)
> > +		return -EINVAL;
> > +
> > +	data = idt_nt_read(ndev, reg) | (u32)set_bits;
> > +	idt_nt_write(ndev, IDT_NT_INDBELLMSK, data);
> 
> Following this function call via itd_ntb_db_set_mask(), it does not appear that the register update is atomic here.  Two threads could read the same old register value and modify it differently, and the second write back to the register would clobber the first.
> 
> In the ntb_hw_intel driver there is a similar setting bits of the doorbell mask, and clearing bits.  Instead of reading the register, modifying it, and writing it back, the current value of the register is stored in memory.  With a spin lock held, the value is updated in memory and then written to the register.  That makes the update atomic, because the spin lock is held through the update and issuing the write, and it saves a trip to read the register.
> 

Great! Thanks for pointing it out. Of course, I'll make it atomic to prevent
the possible race condition.

Additional thanks for drawing my attention to this peace of code, since it's
actually got a "copy-paste" bug. The write method is called with constant
register address, but must be called with "reg" argument.

The same things are going to be fixed in the next idt_reg_clear_bits() method
as well. =)

> 
> > +/*
> > + * idt_get_mw_type() - get memory window size
> 
> Doc doesn't match the function name.
> 
> > + * @mw_type:	Memory window type
> > + *
> > + * Return: number of memory windows corresponding to the type
> 
> This is more like a "count" than a "size".
> 

Agreed.

> > + */
> > +static inline unsigned char idt_get_mw_size(enum idt_mw_type mw_type)
> > +{
> 
> 
> > +/*
> > + * idt_ntb_db_set_mask() - set bits in the local doorbell mask
> > + *			   (NTB API callback)
> > + * @ntb:	NTB device context.
> > + * @db_bits:	Doorbell mask bits to set.
> > + *
> > + * The inbound doorbell register mask value must be read, then OR'ed with
> > + * passed field and only then set back.
> > + *
> > + * Return: zero on success, negative error if invalid argument passed.
> > + */
> > +static int idt_ntb_db_set_mask(struct ntb_dev *ntb, u64 db_bits)
> > +{
> > +	struct idt_ntb_dev *ndev = to_ndev_ntb(ntb);
> > +
> > +	return idt_reg_set_bits(ndev, IDT_NT_INDBELLMSK, IDT_DBELL_MASK,
> > +				db_bits);
> 
> As noted above, this does not appear to be atomic.
> 

Ok.

Thanks for the careful review. The issues will be fixed within the next
patch.

Regards,
-Sergey