On Mon, Mar 25, 2002 at 11:10:27PM -0500, Jeff Garzik wrote:
> christophe barbé wrote:
>
> >On Mon, Mar 25, 2002 at 07:57:19PM -0500, Jeff Garzik wrote:
> >
> >>This patch causes module defaults to be reused -- potentially incorrectly.
> >>
> >
> >Wrong. How can the fact that a suspend/resume cycle increment the id be
> >worst than the fact that the same cycle return idx to the previous
> >state?
> >
> >The argument you have against this patch is WRONG.
> >
> >You think about NICs in a PCI slot.
> >That's changed the day the cardbus support was moved from pcmcia to the
> >today implementation.
> >You can't expect cardbus user to stop using the suspend mode because you
> >expect your id to be attributed one time (that doesn't even make sense).
> >
> >I agree that this patch is not a full fix (I said it in my original
> >post) but I disagree that it does any bad things. I would be interested
> >to learn about a real case ?
> >
>
> Just exactly what I described.
>
> The current system increments the card id on each ->probe, and does not
> decrement on ->remove, which makes sense if you are hotplugging one card
> and then another -- you don't want to reuse the same module options for
> a different card. Your patch changes this logic to, "oh wait, let's
> stop doing this and have a special case once we reach MAX_UNITS" Thus,
> you could potentially reuse the final slot when that was not the desired
> action.
Ok I really appreciate your work but here I can't believe I read what I
read. Please reread the patch :
+ if (vp->card_idx == vortex_cards_found - 1)
+ vortex_cards_found--;
It is NOT a special case when we reach MAX_UNITS.
It is a special case when we remove the last inserted card.
Even in your case where the order is important it still works.
vortex_cards_found is used to attribute the next ID.
> Note that I am not defending this method of card_idx usage, because
> different use cases have different requirements (as indeed you do). But
> your patch fixes one thing at the expense of another.
No and I hope you will agree with me now.
It fixes one thing at the expense of nothing.
> I just had another idea. Create a new module option 'default_options',
> a single integer value. Instead of assigning "-1" to options when we
> run out of MAX_UNITS ids, assign the default_options value.
This would be bloat code.
IMHO MAX_UNITS is a proof of unadequate design here.
Options should be set on a per-device basis by a userland tool which has
the required information to set them in a persistent way.
Is it not the purpose of ethtool ?
By the way the only problem in this tool is its name. It solves a
general problem for a particular subset : network device. Why not the
same tool for all modules ?
I will try to ethtoolize the vortex driver.
> >>This is a personal solution, that might live on temporary as an
> >>outside-the-tree patch... but we cannot apply this to the stable kernel.
> >>
> >>I agree the card idx is wrong on remove. Insert and remove a 3c59x
> >>cardbus card several times, and you will lose your module options too.
> >>
> >
> >NO -- If I can remove/insert suspend/remove my card as I want I ever get
> >the same ID.
> >
> "same id" is vague. Same PCI id? Sure, but that doesn't mean it's the
> same card, from the driver's point of view. The driver really needs to
> keep track of whether or not a new ->probe indicates a card whose MAC
> address we have seen before.
Here by 'same' ID I mean the same id used in the driver to attribute the
option. But we agree (I hope) that it is not a good way to attribute
options to a given card.
> To reiterate another issue, however, suspend/resume should _not_ be
> calling the code added in your patch. That's a non-3c59x bug somewhere.
Good Point.
Christophe
> Jeff
-- Christophe Barbé <christophe.barbe@ufies.org> GnuPG FingerPrint: E0F6 FADF 2A5C F072 6AF8 F67A 8F45 2F1E D72C B41EA qui sait comprendre, peu de mots suffisent. (Intelligenti pauca.)
This archive was generated by hypermail 2b29 : Sun Mar 31 2002 - 22:00:10 EST