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

[Takashi Iwai <tiwai@suse.de>]

At Mon, 8 Aug 2011 17:33:40 +0800,
Wang Shaoyan wrote:
> 
> 2011/8/8 Takashi Iwai <tiwai@suse.de>:
> > At Mon,  8 Aug 2011 16:07:12 +0800,
> > stufever@gmail.com wrote:
> >>
> >> From: Wang Shaoyan <wangshaoyan.pt@taobao.com>
> >>
> >>   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@taobao.com>
> >> ---
> >>  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