Re: [PATCH] 3c59x and resume

["christophe barbé" <christophe.barbe.ml@online.fr>]

[-- Attachment #1: Type: text/plain, Size: 4127 bytes --]

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 B41E

A qui sait comprendre, peu de mots suffisent.
(Intelligenti pauca.) 

[-- Attachment #2: Type: application/pgp-signature, Size: 241 bytes --]