Re: [PATCH] sound: don't use the deprecated function check_region

[Takashi Iwai]

At Mon, 8 Aug 2011 17:33:40 +0800,
Wang Shaoyan wrote:
> 
> 2011/8/8 Takashi Iwai <tiwai@xxxxxxx>:
> > At Mon, Â8 Aug 2011 16:07:12 +0800,
> > stufever@xxxxxxxxx wrote:
> >>
> >> From: Wang Shaoyan <wangshaoyan.pt@xxxxxxxxxx>
> >>
> >> Â sound/oss/pss.c: In function 'configure_nonsound_components':
> >> Â sound/oss/pss.c:676: warning: 'check_region' is deprecated (declared at include/linux/ioport.h:201)
> >>
> >> Signed-off-by: Wang Shaoyan <wangshaoyan.pt@xxxxxxxxxx>
> >> ---
> >> Âsound/oss/pss.c | Â Â3 ++-
> >> Â1 files changed, 2 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/sound/oss/pss.c b/sound/oss/pss.c
> >> index 9b800ce..0fa26bd 100644
> >> --- a/sound/oss/pss.c
> >> +++ b/sound/oss/pss.c
> >> @@ -673,9 +673,10 @@ static void configure_nonsound_components(void)
> >>
> >> Â Â Â if (pss_cdrom_port == -1) { Â Â /* If cdrom port enablation wasn't requested */
> >> Â Â Â Â Â Â Â printk(KERN_INFO "PSS: CDROM port not enabled.\n");
> >> - Â Â } else if (check_region(pss_cdrom_port, 2)) {
> >> + Â Â } else if (!request_region(pss_cdrom_port, 2, "PSS CDROM")) {
> >> Â Â Â Â Â Â Â printk(KERN_ERR "PSS: CDROM I/O port conflict.\n");
> >> Â Â Â } else {
> >> + Â Â Â Â Â Â release_region(pss_cdrom_port, 2);
> >> Â Â Â Â Â Â Â set_io_base(devc, CONF_CDROM, pss_cdrom_port);
> >> Â Â Â Â Â Â Â printk(KERN_INFO "PSS: CDROM I/O port set to 0x%x.\n", pss_cdrom_port);
> >> Â Â Â }
> >
> > Well, this just hides the warning but doesn't fix the possible race,
> > which is the reason check_region() is marked as deprecated after all.
> >
> 
> AFAIK, check_region() is deprecated because such usage:
> check_region()
> ...do something    <-- here another process may do check and request same region
> request_region()
> But nowadays, request_region has done the check internally, so use the
> proper request_region to check is enough, am I miss something?

Right, if it uses request_region() and keeps the resource until the
end of the driver life, it's fine.

> > Instead of releasing the resource there, keep it and release in the
> > destructor properly like other normal resources.
> >
> 
> maybe we can release here?
> 
> set_io_base(devc, CONF_CDROM, pss_cdrom_port);
> printk(KERN_INFO "PSS: CDROM I/O port set to 0x%x.\n", pss_cdrom_port);
> release_region(pss_cdrom_port, 2);

No.  What happens if another driver takes this resource and starts
accessing?  It conflicts.  The driver must keep the resource by itself
to protect from others as long as it uses.


thanks,

Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/