Re: [PATCH 01/14] edac: rewrite the sysfs code to use struct device
From: Greg K H
Date: Fri Mar 30 2012 - 11:32:35 EST
On Fri, Mar 30, 2012 at 07:46:14AM -0300, Mauro Carvalho Chehab wrote:
> Em 30-03-2012 06:11, Borislav Petkov escreveu:
> > On Thu, Mar 29, 2012 at 11:13:07PM -0300, Mauro Carvalho Chehab wrote:
> >> However, every time this patch series is submitted, someone come up with a
> >> bright idea to ask me to add more work to the scope, delaying its addition
> >> forever.
> >> While I'm not convinced that moving from a single memory allocation into a
> >> series of k*alloc is a good thing for a subsystem that is there to detect
> >> memory errors (as having everything altogether into a single page can
> >> reduce the chances of errors at the EDAC data), I can work latter on a
> >> patchset to fix this issue for EDAC MC, but I'll do it only after merging
> >> this series, as it is counter-productive to do it otherwise, having to
> >> repeat the same set of tests on 10 machines (and compile the entire series
> >> of patches on 8 different archs/sub-archs).
> >> So, I really want to move this ahead. So, please, first things first: let's
> >> first fix the more serious bug. Then, we can fix the other minor stuff
> >> that aren't so far causing any noticeable harm.
> > Dude, stop complaining - this is the kernel not some pet project of
> > yours. You either do things right or you don't do them at all. Others
> > have to do the same iterations with patches and intergrate maintainer
> > change requests until everything is done properly.
> > Btw, this patch is
> > 5 files changed, 432 insertions(+), 717 deletions(-)
> > It is 1500+ lines and huuuuge! How do you think anyone can review this?
> If you consider this a big patch, you can imagine how bigger it will be if
> it would have there the re-write the edac_mc_alloc() function, in order to break
> it into a per-struct-device function, likely patching all drivers/edac/*.c
> to use the new way.
> As I said: merging the allocation fix on this patch is a very bad idea:
> it should be a separate changeset, applied after this one, as the
> subsequent changesets simplify the sysfs logic, helping to write a changeset
> to fix the kobject issue.
> Applying it before would just do a lot of changes on some code that will
> be dropped by this series, making harder for busy reviewers to inspect
> the changes.
> So, as I said, the way to move is to apply this changeset, and then to
> go ahead and cleanup the potential problem  of having multiple kobj
> references for the same memory block.
>  I never saw any bugzilla complaining about an EDAC failure due to the
> usage of multiple kobjects at the same memory block. The reason is probably
> because, in practice, once this module is loaded to monitor the memory errors,
> this module is never unloaded. Also, module unload/reload works, before
> and after this changeset. So, AFAIKT, nobody ever noticed this existing
> bug before yesterday.
> > Also, I told already: if you wanna fix one thing, then fix it with a
> > smaller patchset which easier to review by people instead of throwing at
> > them humongous patch bombs which are supposed to fix _everything_ and
> > expecting everyone to understand immediately what you mean. And don't
> > tell me these huge patches cannot be split, I'm not buying it.
> This patch does the absolute minimum stuff to replace kobj by struct device.
> Nothing more, nothing less.
> I took the care to put all needed driver changes and API changes
> on separate patchsets.
> The edac_mc_sysfs.c file has just one thing there: the sysfs logic, based
> on kobj raw allocation.
> Replacing it by struct device means to rewrite the entire code. Period.
> Breaking it into smaller pieces would break git bisect, and will make it
> even harder for reviewers, as this atomic change unit would be broken
> into several patches.
> Btw, both Greg and Aris reviewed it yesterday. So, it seems that this is
> not as complex as you think.
No, it's very complex, I just searched for the most obvious problem that
people do with using 'struct device' and reported on that problem. I
didn't look at anything else.
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/