Re: [PATCH 30/57] power: ab8500: Flush & sync all works

[Anton Vorontsov]

On Fri, Sep 28, 2012 at 12:28:03PM -0600, Mathieu Poirier wrote:
[...]
> >> +	flush_delayed_work_sync(&di->attach_work);
> >> +	flush_delayed_work_sync(&di->usb_charger_attached_work);
> >> +	flush_delayed_work_sync(&di->ac_charger_attached_work);
> >> +	flush_delayed_work_sync(&di->check_usbchgnotok_work);
> >> +	flush_delayed_work_sync(&di->check_vbat_work);
> >> +	flush_delayed_work_sync(&di->kick_wd_work);
> >> +
> >> +	flush_work_sync(&di->usb_link_status_work);
> >> +	flush_work_sync(&di->ac_work);
> >> +	flush_work_sync(&di->detect_usb_type_work);
> > 
> > I belive each of these have to be added by the patches that add the
> > appropriate work structs. But really, it's better to avoid these many
> > delayed work.
> 
> Agreed - on the flip side they were added over multiple patches spanning
> many centuries.  Tracking all this work down would be impossible as
> history was re-written many time over.  What would you like to see
> happening here ?

Um.. why can't we just fold

+	flush_delayed_work_sync(&di->attach_work);

Into "[PATCH 20/57] power: Adds support for legacy USB chargers"?

Then 

+	flush_delayed_work_sync(&di->usb_charger_attached_work);
+	flush_delayed_work_sync(&di->ac_charger_attached_work);

Into "[PATCH 07/57] power: ab8500_bm: Detect removed charger"

etc...

That is, at the same time you introduce the work structs, you have to
flush them appropriately.

For work structs that were already in mainline, you'll have this patch to
fixup things. But adding new code with known bugs, and then fixing them in
one go is usually not how things should be done (unless there's a good
technical reason behind this).

Thanks,
Anton.
--
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/