Re: [PATCH 0/5 v2] PCI: fix cardbus and sriov regressions

[Linus Torvalds]

Gaah. I'm still rather uncomfortable about this, and I wonder about
patch 2 in particular. It seems that that patch could/should be split
up: the whole change to "find_resource()" etc looks like prime
material for a separate patch that splits up that function and
explains why that change is done.

Also, quite frankly, by the time you pass in eight different arguments
(and pretty complex ones at that, with one being the alignment
function pointer), I start thinking that you should have passed in a
pointer to a descriptor structure instead. I get the feeling that the
"resource requirements" really should be a structure instead of lots
of individual arguments:

IOW, this part:

+                     resource_size_t newsize, resource_size_t min,
+                     resource_size_t max, resource_size_t align,
+                     resource_size_t (*alignf)(void *,
+                                               const struct resource *,
+                                               resource_size_t,
+                                               resource_size_t),
+                     void *alignf_data)

really makes me go

   struct resource_requirement {
      resource_size_t min, max, align;
      resource_size_t (*alignf)(const struct resource *, struct
resource_requirement *);
      void *alignf_data);
   };

and I'd really change the function argument to take that kind of
simplified thing instead.

And that cleanup/re-organization would be prime material for a totally
independent patch that changes no semantics at all, just prepares for
the other changes.

That way the final "patch 2" would be smaller and do the semantic
changes, instead of being a mix of semantic changes and infrastructure
changes.

And some of the cleanup stuff I could merge for 3.0 just to make things easier.

Hmm?

                 Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/