[ofa-general] RE: [PATCH v2] RDMA/nes: Mitigate compatibility issue regarding PCI write credits

[Tung, Chien Tin]

> > +module_param(limit_maxrdreqsz, int, 0644);
>
>type can be bool instead of int here?

Yes, it should be bool.

> > +	if ((limit_maxrdreqsz) ||
> > +		((nesdev->nesadapter->phy_type[0] == 
>NES_PHY_TYPE_GLADIUS) &&
> > +		(hw_rev == NE020_REV1))) {
> > +		nes_debug(NES_DBG_INIT,
>
>This indentation is hard to read, because the then clause visually runs
>into the condition being tested.  I generally align the follow-on lines
>to be just inside the opening ( of "if (".  And there's no 
>reason to put
>parentheses around limit_maxrdreqsz...

I normally don't indent that way either but CodingStyle doc said I
can't use spaces to indent...

"Outside of comments, documentation and except in Kconfig, spaces are never
used for indentation, and the above example is deliberately broken."

> > +		pci_read_config_word(pcidev, 0x68, &maxrdreqword);
> > +		/* set bits 12-14 to 001b = 256 bytes */
> > +		maxrdreqword &= 0x8fff;
> > +		maxrdreqword |= 0x1000;
> > +		pci_write_config_word(pcidev, 0x68, maxrdreqword);
>
>I would write this as below, using the standard pcie 
>interfaces and also
>being defensive so as not to set the max read req to 256 if the
>BIOS/kernel had limited it to 128 already:
>
>	if (pcie_get_readrq(pcidev) > 256)
>		if (pcie_set_readrq(pcidev, 256)) {
>			/* report error */
>		}

Thanks for the change.  Want a v3 for this patch?

Chien