[alsa-devel] [PATCH 1/1] ALSA: Fix limit of 8 PCM devices in SNDRV_CTL_IOCTL_PCM_NEXT_DEVICE

[Takashi Iwai]

At Wed, 30 Jul 2008 11:45:48 +0100,
Pawel MOLL wrote:
> 
> > Could you split this as a separate patch?  This unsigned int -> int
> > change is irrelevant with the fix for 8 PCM devices limit.
> 
> Well, actually it is :-) Take a look here:
> 
> static inline int snd_pcm_next(struct snd_card *card, int device)
> {
>         struct snd_pcm *pcm;
> 
>         list_for_each_entry(pcm, &snd_pcm_devices, list) {
>                 if (pcm->card == card && pcm->device > device)
>                                          ^^^^^^^^^^^^^^^^^^^^
>                         return pcm->device;
>                 else if (pcm->card->number > card->number)
>                         return -1;
>         }
>         return -1;
> }

Ah yeah, this new part contains it.

> 
> When device = -1 (first step on enumeration) the expression:
> 
>   (unsigned)pcm->device > (unsigned)device
> 
> will be always 0 as (unsigned)-1 == 0xffffffff...
>
> I wouldn't touch this at all in other case ;-)
> 
> > You cannot use SNDRV_OS_MINORS unconditionally here.
> > In the case DYNAMIC_MINORS=n, SNDRV_PCM_DEVICES is still valid.  Maybe
> > better to define SNDRV_PCM_DEVICES with ifdef, such as,
> > 
> > #ifdef CONFIG_SND_DYNAMIC_MINORS
> > #define SNDRV_PCM_DEVICES	(SNDRV_OS_MINORS-2)
> > #else
> > #define SNDRV_PCM_DEVICES	8
> > #endif
> 
> To be perfectly honest I don't see the point, as in both cases the value
> may be simply wrong (I mean device not existing at all), so as for me
> these checks are just "safety fuses" and above is yet another #ifdef,
> but no problem - I will revive this constant.

Yes, it's for safety reason.  And, no reason to break old behavior.
In slightest case, it can be a regression :)

> > > -static struct snd_pcm *snd_pcm_search(struct snd_card *card, int
> > device)
> > > +static inline struct snd_pcm *snd_pcm_get(struct snd_card *card,
> > int device)
> > 
> > Don't use inline.  It's good for a single-line or a pretty small
> > function, but not for else.  The compiler is clever enough to inline
> > the functions automatically if needed.
> 
> Well, I used to define functions which are called from just one place
> (which are usually just an extracted part of other huge function ;-) as
> inline, but as you wish - I wont persist on it.

Especially in such a case, you really don't have to make it inline.

> > Also, any reason to rename to *_get()?  get() is usually paired with
> > put().
> 
> I see your point. My motivation was: this function is not used for
> searching the list any more, just for getting a reference to one,
> defined element. Of course I can leave it as it was.

OK, point taken.  No big problem to rename, but I just wondered.


thanks,

Takashi