[ofa-general] Re: RFC: modify upstream code to make backporting easier

[Ralph Campbell]

Looks OK to me from the InfiniPath side.
Keeping the backported code as close as possible to the
upstream code is a good thing in my view.

On Tue, 2007-09-11 at 09:28 +0300, Michael S. Tsirkin wrote:
> Roland, Ralph, all,
> I'd like to get your opinion on the following matter:
> OFED is backporting upstream rdma code to older kernels.
> While doing so, I really take pains to keep the ported
> code as close as possible to upstream original,
> mostly by using preprocessor to implement, as closely
> as possible, the APIs from recent kernels on top of
> older ones.
> 
> As an example where this works well, see my backport of the
> new workqueue API to 2.6.19:
> http://www.openfabrics.org/git/?p=ofed_1_3/linux-2.6.git;a=blob;f=kernel_addons/backport/2.6.19/include/linux/workqueue.h;hb=HEAD
> 
> However, sometimes I am forced to patch the upstream code. Here's an
> example of the patch needed to make ipath build on
> 2.6.22:
> 
> 
> diff --git a/drivers/infiniband/hw/ipath/ipath_driver.c b/drivers/infiniband/hw/ipath/ipath_driver.c
> index 09c5fd8..94edb5d 100644
> --- a/drivers/infiniband/hw/ipath/ipath_driver.c
> +++ b/drivers/infiniband/hw/ipath/ipath_driver.c
> @@ -287,6 +287,7 @@ static int __devinit ipath_init_one(struct pci_dev *pdev,
>  	struct ipath_devdata *dd;
>  	unsigned long long addr;
>  	u32 bar0 = 0, bar1 = 0;
> +	u8 rev;
>  
>  	dd = ipath_alloc_devdata(pdev);
>  	if (IS_ERR(dd)) {
> @@ -448,7 +449,13 @@ static int __devinit ipath_init_one(struct pci_dev *pdev,
>  	dd->ipath_deviceid = ent->device;	/* save for later use */
>  	dd->ipath_vendorid = ent->vendor;
>  
> -	dd->ipath_pcirev = pdev->revision;
> +	ret = pci_read_config_byte(pdev, PCI_REVISION_ID, &rev);
> +	if (ret) {
> +		ipath_dev_err(dd, "Failed to read PCI revision ID unit "
> +			      "%u: err %d\n", dd->ipath_unit, -ret);
> +		goto bail_regions;	/* shouldn't ever happen */
> +	}
> +	dd->ipath_pcirev = rev;
>  
>  #if defined(__powerpc__)
>  	/* There isn't a generic way to specify writethrough mappings */
> 
> 
> As you can see, there's nothing I can do with macros outside the code
> to make it work without code changes.
> However, the patching mechanism is pretty fragile with respect
> to code reorgs etc.
> I wonder whether it's acceptable in cases such as this to add
> a wrapper in upstream code. For example, upstream could have:
> 
> #ifndef pci_get_revision
> #define pci_get_revision(dev) ((dev)->revision)
> #endif
> 
> and then all a 2.6.22 backport needs to do is define it's own
> pci_get_revision macro.
> 
> Upstream maintainers, can you pls comment ASAP on whether such approach would be
> acceptable e.g. for 2.6.24?  If I could get rid of backport
> patches, it might make sense to start thinking about converting fixes
> patches to git commits, post 1.3, as well.
> 
> Thanks,
>