Re: [alsa-devel] SB16 build error.
On Thu, Jun 30, 2011 at 01:28:03PM +0200, Takashi Iwai wrote:
I have no idea how big the soundblaster microcode being loaded actually is, that is if the reduced size of 0x1f00 will be sufficient.
The files found in /lib/firmware/sb16 are all under 2kB, thus likely sufficient.
Too shortly answered. It turned out that some CSP codes (like Qsound) can be above that size, it's almost 12kB. So the size in the original code is really the necessary requirement, and the patch breaks for such a case.
An ugly workaround would be to fake the ioctl size. But this is certainly to be avoided, since it has been broken on the specific platforms for ages, thus breaking for them would be mostly harmless, too.
Aside of that I don't see a problem - I don't see how the old ioctl can possibly have been used before so there isn't a compatibility problem.
Or you could entirely sidestep the problem and use request_firmware() but I guess that's more effort than you want to invest.
Yeah, that's another option I thought of. But it's too intrusive for 3.0-rc6, so I'd like waive it for 3.1.
Actually the request_firmware() was implemented for some auto-loadable CSP codes. Others need the manual loading, so it is via ioctl. It can be converted, but I don't think it makes sense for such old stuff. After all, it still works with x86-ISA as is.
In userland an empty definition will be used for _IOC_TYPECHECK so there won't be an error. So userland already is already using the existing value for SNDRV_SB_CSP_IOCTL_LOAD_CODE ...
With a crude hack like
#define SNDRV_SB_CSP_IOCTL_LOAD_CODE \ _IOC(_IOC_WRITE,'H', 0x11, sizeof(struct snd_sb_csp_microcode))
error checking can be bypassed and all will be fine as long as the resulting value doesn't result in in a a duplicate case value - which it doesn't, at least not in my testing.
Should work but isn't nice.
Ralf
At Thu, 30 Jun 2011 13:32:12 +0100, Ralf Baechle wrote:
On Thu, Jun 30, 2011 at 01:28:03PM +0200, Takashi Iwai wrote:
I have no idea how big the soundblaster microcode being loaded actually is, that is if the reduced size of 0x1f00 will be sufficient.
The files found in /lib/firmware/sb16 are all under 2kB, thus likely sufficient.
Too shortly answered. It turned out that some CSP codes (like Qsound) can be above that size, it's almost 12kB. So the size in the original code is really the necessary requirement, and the patch breaks for such a case.
An ugly workaround would be to fake the ioctl size. But this is certainly to be avoided, since it has been broken on the specific platforms for ages, thus breaking for them would be mostly harmless, too.
Aside of that I don't see a problem - I don't see how the old ioctl can possibly have been used before so there isn't a compatibility problem.
Or you could entirely sidestep the problem and use request_firmware() but I guess that's more effort than you want to invest.
Yeah, that's another option I thought of. But it's too intrusive for 3.0-rc6, so I'd like waive it for 3.1.
Actually the request_firmware() was implemented for some auto-loadable CSP codes. Others need the manual loading, so it is via ioctl. It can be converted, but I don't think it makes sense for such old stuff. After all, it still works with x86-ISA as is.
In userland an empty definition will be used for _IOC_TYPECHECK so there won't be an error. So userland already is already using the existing value for SNDRV_SB_CSP_IOCTL_LOAD_CODE ...
Right. It has an invalid direction (3), but apps won't care such details anyway.
With a crude hack like
#define SNDRV_SB_CSP_IOCTL_LOAD_CODE \ _IOC(_IOC_WRITE,'H', 0x11, sizeof(struct snd_sb_csp_microcode))
error checking can be bypassed and all will be fine as long as the resulting value doesn't result in in a a duplicate case value - which it doesn't, at least not in my testing.
Should work but isn't nice.
Indeed. But which is uglier is hard to answer :)
If you are fine with the hacked ioctl number above, I can put it with some comments. This won't break anything, at least.
thanks,
Takashi
On Thu, Jun 30, 2011 at 02:38:20PM +0200, Takashi Iwai wrote:
In userland an empty definition will be used for _IOC_TYPECHECK so there won't be an error. So userland already is already using the existing value for SNDRV_SB_CSP_IOCTL_LOAD_CODE ...
Right. It has an invalid direction (3), but apps won't care such details anyway.
With a crude hack like
#define SNDRV_SB_CSP_IOCTL_LOAD_CODE \ _IOC(_IOC_WRITE,'H', 0x11, sizeof(struct snd_sb_csp_microcode))
error checking can be bypassed and all will be fine as long as the resulting value doesn't result in in a a duplicate case value - which it doesn't, at least not in my testing.
Should work but isn't nice.
Indeed. But which is uglier is hard to answer :)
If you are fine with the hacked ioctl number above, I can put it with some comments. This won't break anything, at least.
Go ahead then and yes, this really deserves a comment.
Ralf
At Thu, 30 Jun 2011 13:43:33 +0100, Ralf Baechle wrote:
On Thu, Jun 30, 2011 at 02:38:20PM +0200, Takashi Iwai wrote:
In userland an empty definition will be used for _IOC_TYPECHECK so there won't be an error. So userland already is already using the existing value for SNDRV_SB_CSP_IOCTL_LOAD_CODE ...
Right. It has an invalid direction (3), but apps won't care such details anyway.
With a crude hack like
#define SNDRV_SB_CSP_IOCTL_LOAD_CODE \ _IOC(_IOC_WRITE,'H', 0x11, sizeof(struct snd_sb_csp_microcode))
error checking can be bypassed and all will be fine as long as the resulting value doesn't result in in a a duplicate case value - which it doesn't, at least not in my testing.
Should work but isn't nice.
Indeed. But which is uglier is hard to answer :)
If you are fine with the hacked ioctl number above, I can put it with some comments. This won't break anything, at least.
Go ahead then and yes, this really deserves a comment.
OK, here is the patch.
Takashi
=== From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: sb16 - Fix build errors on MIPS and others with 13bit ioctl size
One of ioctl definition in sound/sb16_csp.h contains the data size over 8kB, and this causes build errors on architectures like MIPS, which define _IOC_SIZEBITS=13.
For avoiding this build errors but keeping the compatibility, manually exapnd with _IOC() instead of using _IOW() for the problematic ioctl.
Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/sb16_csp.h | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/include/sound/sb16_csp.h b/include/sound/sb16_csp.h index 736eac7..af1b49e 100644 --- a/include/sound/sb16_csp.h +++ b/include/sound/sb16_csp.h @@ -99,7 +99,14 @@ struct snd_sb_csp_info { /* get CSP information */ #define SNDRV_SB_CSP_IOCTL_INFO _IOR('H', 0x10, struct snd_sb_csp_info) /* load microcode to CSP */ -#define SNDRV_SB_CSP_IOCTL_LOAD_CODE _IOW('H', 0x11, struct snd_sb_csp_microcode) +/* NOTE: struct snd_sb_csp_microcode overflows the max size (13 bits) + * defined for some architectures like MIPS, and it leads to build errors. + * (x86 and co have 14-bit size, thus it's valid, though.) + * As a workaround for skipping the size-limit check, here we don't use the + * normal _IOW() macro but _IOC() with the manual argument. + */ +#define SNDRV_SB_CSP_IOCTL_LOAD_CODE \ + _IOC(_IOC_WRITE, 'H', 0x11, sizeof(struct snd_sb_csp_microcode)) /* unload microcode from CSP */ #define SNDRV_SB_CSP_IOCTL_UNLOAD_CODE _IO('H', 0x12) /* start CSP */
On Thursday 30 June 2011, Ralf Baechle wrote:
#define SNDRV_SB_CSP_IOCTL_LOAD_CODE \ _IOC(_IOC_WRITE,'H', 0x11, sizeof(struct snd_sb_csp_microcode))
error checking can be bypassed and all will be fine as long as the resulting value doesn't result in in a a duplicate case value - which it doesn't, at least not in my testing.
Should work but isn't nice.
Right. It's probably the best we can do. I think we added a few similar definitions when we originally introduce _IOC_TYPECHECK. The idea was never to break existing code, but rather to avoid merging new drivers that use inconsistent ioctl command numbers.
Arnd
participants (3)
-
Arnd Bergmann -
Ralf Baechle -
Takashi Iwai