On Thu, 2007-09-06 at 11:09 -0600, Nicolas Droux wrote:
>
> Garrett D'Amore wrote:
> > On Tue, 2007-09-04 at 12:45 -0700, Thirumalai Srinivasan wrote:
> >> Iam not talking about the asynchronous i_mac_notify_thread(), and
> >> synchronizing that with mac_unregister() is the job of the mac framework.
> >>
> >> Iam saying drivers have to ensure that they are pretty much
> >> single-threaded (with respect to mac calls) while they call
> >> mac_unregister(). Iam hoping that is already the case today.
> >
> > Well, I know at least a couple of drivers that don't obey this rule.
> > (Every driver I've looked at breaks it... e1000g, bge, eri, hme, rtls,
> > mxfe, etc.)
> >
> > This is one problem with having mac_unregister free the mac_impl_t.
> >
> > I think the GLDv2 was better here... it had explicit routines to
> > allocate and free the soft state, independent of the routines to
> > register and unregister. For some reason someone decided to get
> > "clever" with the GLDv3...
>
> We already have mac_alloc() and mac_free() routines. I don't think we
> should have another free function added to the mix. If we require one
> mac_alloc() and mac_free() per registered MAC, then it seems we could
> have them allocate and free the mac_impl_t as well. Instead of invoking
> mac_free() right after mac_unregister(), the drivers would be required
> to invoke mac_free() after removing their interrupts and stopping their
> timers, etc.
Right.
I actually am of the opinion that the separate mac_register_t is a bit
screwball.
IMO, its simpler to follow the old GLDv2 paradigm, where the driver does
mac_alloc(), which allocates the mac_impl_t, the driver populates the
*public portions* of this, and when the driver is done, it can do
mac_free().
One way to make sure that drivers only touch the "public" portions is to
have the public members in their own structure...
struct mac_t {
/* public members here */
}
and then have the "impl" have the mac_t as its first member. Then
casting can be used back and forth.
struct mac_impl_t {
struct mac_t mi_public;
/* follow with private members */
}
(Obviously mac_alloc() here would return "mac_t *", but under the covers
it is really allocating a whole mac_impl_t. And mac_free() would be
free()'ing a a whole mac_impl_t, though it is taking a mac_t as its
argument.)
If we want to pursue this course of action, I can put together a webrev
with the changes... it will take me a while because there are a *lot* of
drivers to change... but I want to get this change in *before* we make
GLDv3 public.
-- Garrett
>
> BTW, I looked at your changes and they look good to me.
>
> Nicolas.
>
_______________________________________________
networking-discuss mailing list
[email protected]
