__check_region in ide code?

__check_region in ide code?

[Rusty Russell]

Hi Bart,

	I notice that drivers/ide/ide-probe.c's hwif_check_region()
still uses check_region().  If it really does want to use it to probe
and not reserve, I think we should stop it warning there.

	There's nothing inherently *wrong* with check_region, it's
just deprecated to trap the old (now racy) idiom of "if
(check_region(xx)) reserve_region(xx)".  There's no reason not to
introduce a probe_region if IDE really wants it.

Of course, some people will start "fixing" drivers by
s/check_region/probe_region/ when we do this, but that's the risk we
take.

It should also allow us to easily get rid of that stupid warning in
ksyms.c...

Thoughts?
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: __check_region in ide code?

[Bartlomiej Zolnierkiewicz]

On Fri, 6 Jun 2003, Rusty Russell wrote:

> Hi Bart,

Hi Rusty,

> 	I notice that drivers/ide/ide-probe.c's hwif_check_region()
> still uses check_region().  If it really does want to use it to probe
> and not reserve, I think we should stop it warning there.
>
> 	There's nothing inherently *wrong* with check_region, it's
> just deprecated to trap the old (now racy) idiom of "if
> (check_region(xx)) reserve_region(xx)".  There's no reason not to
> introduce a probe_region if IDE really wants it.

And ide-probe.c does exactly this racy stuff.

I did patch to convert it to request_region() some time ago,
I just need to double check it and submit.

Thanks,
--
Bartlomiej

> Of course, some people will start "fixing" drivers by
> s/check_region/probe_region/ when we do this, but that's the risk we
> take.
>
> It should also allow us to easily get rid of that stupid warning in
> ksyms.c...
>
> Thoughts?
> Rusty.
> --
>   Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: __check_region in ide code?

[Alan Cox]

On Gwe, 2003-06-06 at 09:56, Bartlomiej Zolnierkiewicz wrote:
> > 	There's nothing inherently *wrong* with check_region, it's
> > just deprecated to trap the old (now racy) idiom of "if
> > (check_region(xx)) reserve_region(xx)".  There's no reason not to
> > introduce a probe_region if IDE really wants it.
> 
> And ide-probe.c does exactly this racy stuff.
> 
> I did patch to convert it to request_region() some time ago,
> I just need to double check it and submit.

request_region at that point doesn't actually help you. For PIO devices
its too late if you are handling PCMCIA, for PCI devices its too late
because you want to own the PCI device properly, for MMIO its completely
broken (all the mem region stuff in 2.5)

The only way I can see to fix it properly is to provide ide helpers
for resource allocation that are used by the drivers when needed.

Re: __check_region in ide code?

[Bartlomiej Zolnierkiewicz]

On 6 Jun 2003, Alan Cox wrote:

> On Gwe, 2003-06-06 at 09:56, Bartlomiej Zolnierkiewicz wrote:
> > > 	There's nothing inherently *wrong* with check_region, it's
> > > just deprecated to trap the old (now racy) idiom of "if
> > > (check_region(xx)) reserve_region(xx)".  There's no reason not to
> > > introduce a probe_region if IDE really wants it.
> >
> > And ide-probe.c does exactly this racy stuff.
> >
> > I did patch to convert it to request_region() some time ago,
> > I just need to double check it and submit.
>
> request_region at that point doesn't actually help you. For PIO devices
> its too late if you are handling PCMCIA, for PCI devices its too late
> because you want to own the PCI device properly, for MMIO its completely
> broken (all the mem region stuff in 2.5)

Yes, I am aware of that.
Patch is only to fix ide-probe.c (request_region() after check_region())
not whole ide resource allocation braindamage.

> The only way I can see to fix it properly is to provide ide helpers
> for resource allocation that are used by the drivers when needed.

Exactly, it is already on my todo.
--
Bartlomiej

Re: __check_region in ide code?

[William Lee Irwin III]

On Fri, Jun 06, 2003 at 05:34:24PM +1000, Rusty Russell wrote:
> 	I notice that drivers/ide/ide-probe.c's hwif_check_region()
> still uses check_region().  If it really does want to use it to probe
> and not reserve, I think we should stop it warning there.
> 	There's nothing inherently *wrong* with check_region, it's
> just deprecated to trap the old (now racy) idiom of "if
> (check_region(xx)) reserve_region(xx)".  There's no reason not to
> introduce a probe_region if IDE really wants it.
> Of course, some people will start "fixing" drivers by
> s/check_region/probe_region/ when we do this, but that's the risk we
> take.
> It should also allow us to easily get rid of that stupid warning in
> ksyms.c...

I've actually seen IDE oops doing a racy check_region()/request_region()
on 2.4.x-test*. Either the fix I brewed up never hit mainline or there's
more than the one I hit.


-- wli