Can we remove pci_find_device() yet?

Am 08.01.2010 05:46 schrieb Greg KH:
> On Fri, Jan 08, 2010 at 11:22:36AM +1100, Stephen Rothwell wrote:
>>
>> This interface has been deprecated since November 2006, so can something
>> be done about the last users (I think only some ISDN drivers)?

That would be the HiSax I4L drivers.

> Ick, the "new" isdn drivers are still not merged that replace those
> older ones?  I thought that happened a while ago.

mISDN, which is the designated successor to HiSax, has indeed been
merged in 2008.

Trouble is, mISDN's userspace interface is completely different from I4L.
Removing HiSax would therefore break userspace compatibility for current
users of HiSax supported devices. In order to switch to mISDN they'll
have to replace all of their ISDN4Linux applications with equivalent
mISDN ones, and I'm not even sure there is a replacement for everything.

Just an idea - as a stopgap measure, couldn't pci_find_device() be made
a private function of the HiSax drivers? That way, the remainder of the
kernel won't be polluted by it anymore, and the PCI_LEGACY config option
can be dropped. Something like this quick and dirty hack:

On Mon, Jan 11, 2010 at 08:46:50PM +0100, Tilman Schmidt wrote:
> Am 08.01.2010 05:46 schrieb Greg KH:
> > On Fri, Jan 08, 2010 at 11:22:36AM +1100, Stephen Rothwell wrote:
> >>
> >> This interface has been deprecated since November 2006, so can something
> >> be done about the last users (I think only some ISDN drivers)?
> 
> That would be the HiSax I4L drivers.
> 
> > Ick, the "new" isdn drivers are still not merged that replace those
> > older ones?  I thought that happened a while ago.
> 
> mISDN, which is the designated successor to HiSax, has indeed been
> merged in 2008.
> 
> Trouble is, mISDN's userspace interface is completely different from I4L.
> Removing HiSax would therefore break userspace compatibility for current
> users of HiSax supported devices. In order to switch to mISDN they'll
> have to replace all of their ISDN4Linux applications with equivalent
> mISDN ones, and I'm not even sure there is a replacement for everything.
> 
> Just an idea - as a stopgap measure, couldn't pci_find_device() be made
> a private function of the HiSax drivers? That way, the remainder of the
> kernel won't be polluted by it anymore, and the PCI_LEGACY config option
> can be dropped. Something like this quick and dirty hack:

Close, but if you do this, please name the function
hisax_find_pci_device() or something, and change the drivers to use it
instead.  Also put a big fat warning in the function that calling this
is unsafe for any PCI hotplug type machine.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Greg,

On Mon, 11 Jan 2010 12:01:36 -0800 Greg KH <gregkh@suse.de> wrote:
>
> Close, but if you do this, please name the function
> hisax_find_pci_device() or something, and change the drivers to use it
> instead.  Also put a big fat warning in the function that calling this
> is unsafe for any PCI hotplug type machine.

There is another implementation of pci_find_device in
drivers/staging/dt3155/pci-compat.h (used by dt3155_drv.c).

On Tue, Jan 12, 2010 at 11:21:15AM +1100, Stephen Rothwell wrote:
> Hi Greg,
> 
> On Mon, 11 Jan 2010 12:01:36 -0800 Greg KH <gregkh@suse.de> wrote:
> >
> > Close, but if you do this, please name the function
> > hisax_find_pci_device() or something, and change the drivers to use it
> > instead.  Also put a big fat warning in the function that calling this
> > is unsafe for any PCI hotplug type machine.
> 
> There is another implementation of pci_find_device in
> drivers/staging/dt3155/pci-compat.h (used by dt3155_drv.c).

That file will be going away soon and the whole driver will be ported to
use the "real" PCI api, so don't worry about breaking any staging
drivers :)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 2010-01-11 21:01 schrieb Greg KH:
> On Mon, Jan 11, 2010 at 08:46:50PM +0100, Tilman Schmidt wrote:
>>
>> Just an idea - as a stopgap measure, couldn't pci_find_device() be made
>> a private function of the HiSax drivers? That way, the remainder of the
>> kernel won't be polluted by it anymore, and the PCI_LEGACY config option
>> can be dropped. Something like this quick and dirty hack:
> 
> Close, but if you do this, please name the function
> hisax_find_pci_device() or something, and change the drivers to use it

Sorry, but no. That would drag me into the checkpatch.pl swamp,
a place I know well enough by now to avoid it whenever possible.
Many of the calls to pci_find_device() have checkpatch problems
which of course do not go away by just substituting another
function name, so I would be obliged to restructure all those
call sites by hand for the sake of "not introducing new code
with checkpatch problems". BTDT.

So I'll drop that idea. If someone else wants to pick it up,
feel free to do so.

Regards,
Tilman

- -- 
Tilman Schmidt                    E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.12 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAktMpmYACgkQQ3+did9BuFuJKgCggRImZ3NOTmCJUpUktreervtz
fegAniAexJirz3p/AXPB6EpsCEJn3hPL
=QwUB
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Jan 12, 2010 at 05:42:14PM +0100, Tilman Schmidt wrote:
> Am 2010-01-11 21:01 schrieb Greg KH:
> > On Mon, Jan 11, 2010 at 08:46:50PM +0100, Tilman Schmidt wrote:
> >>
> >> Just an idea - as a stopgap measure, couldn't pci_find_device() be made
> >> a private function of the HiSax drivers? That way, the remainder of the
> >> kernel won't be polluted by it anymore, and the PCI_LEGACY config option
> >> can be dropped. Something like this quick and dirty hack:
> > 
> > Close, but if you do this, please name the function
> > hisax_find_pci_device() or something, and change the drivers to use it
> 
> Sorry, but no. That would drag me into the checkpatch.pl swamp,
> a place I know well enough by now to avoid it whenever possible.

I don't understand, why?

> Many of the calls to pci_find_device() have checkpatch problems
> which of course do not go away by just substituting another
> function name, so I would be obliged to restructure all those
> call sites by hand for the sake of "not introducing new code
> with checkpatch problems". BTDT.

No you would not, don't be crazy.  Modfying a single line to rename a
function from one thing to another could never be decreed as a "don't
add bad code".  And if someone says it is, well, you just found someone
who doesn't know what they are doing :)

> So I'll drop that idea. If someone else wants to pick it up,
> feel free to do so.

Please continue with this idea, it's sane, and valid.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Greg KH <gregkh@suse.de> writes:

> Close, but if you do this, please name the function
> hisax_find_pci_device() or something, and change the drivers to use it
> instead.  Also put a big fat warning in the function that calling this
> is unsafe for any PCI hotplug type machine.

BTW, I have a driver in the works which uses these functions, and I
wonder how to do it correctly. The problem is that the hardware is a
dual-function PCI card (two DEC Tulip Ethernet controllers) but part of
the first device (EEPROM interface or something like that) is used by
both (there is an FPGA connected there, serving both channels).

This means drivers for dev#0 and dev#1 (both "normal" PCI controllers)
need to register and access part of dev#0 BARs.

How do I do that properly, in terms of PCI API?

On Tue, 2010-01-12 at 21:34 +0100, Krzysztof Halasa wrote:
> Greg KH <gregkh@suse.de> writes:
> 
> > Close, but if you do this, please name the function
> > hisax_find_pci_device() or something, and change the drivers to use it
> > instead.  Also put a big fat warning in the function that calling this
> > is unsafe for any PCI hotplug type machine.
> 
> BTW, I have a driver in the works which uses these functions, and I
> wonder how to do it correctly. The problem is that the hardware is a
> dual-function PCI card (two DEC Tulip Ethernet controllers) but part of
> the first device (EEPROM interface or something like that) is used by
> both (there is an FPGA connected there, serving both channels).
> 
> This means drivers for dev#0 and dev#1 (both "normal" PCI controllers)
> need to register and access part of dev#0 BARs.

I hope you realise that this is a design error!  Linux is fairly
forgiving and allows PCI drivers to do unusual things, but other OSes
make it hard or impossible for PCI drivers to coordinate multiple
functions.

> How do I do that properly, in terms of PCI API?

One of our older controllers had a similar design error in that the two
functions would sometimes have to be reset together.  The way we look
for the second function is:

	dev = pci_dev_get(dev1);
	while ((dev = pci_get_device(vendor_id, device_id, dev))) {
		if (dev->bus == dev1->bus &&
		    dev->devfn == dev1->devfn + 1) {
			dev2 = dev;
			break;
		}
	}

I assume this is 'properly' since no-one has told me otherwise.

Ben.

On Tue, Jan 12, 2010 at 08:53:25PM +0000, Ben Hutchings wrote:
> > How do I do that properly, in terms of PCI API?
> 
> One of our older controllers had a similar design error in that the two
> functions would sometimes have to be reset together.  The way we look
> for the second function is:
> 
> 	dev = pci_dev_get(dev1);
> 	while ((dev = pci_get_device(vendor_id, device_id, dev))) {
> 		if (dev->bus == dev1->bus &&
> 		    dev->devfn == dev1->devfn + 1) {
> 			dev2 = dev;
> 			break;
> 		}
> 	}
> 
> I assume this is 'properly' since no-one has told me otherwise.

As of right now, yes, this is the "correct" way to do this.

I need to dig up the "let multiple drivers bind to a single device"
patch set to make this not necessary in the future.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 2010-01-12 18:39 schrieb Greg KH:
> On Tue, Jan 12, 2010 at 05:42:14PM +0100, Tilman Schmidt wrote:
[...]
>> Many of the calls to pci_find_device() have checkpatch problems
>> which of course do not go away by just substituting another
>> function name, so I would be obliged to restructure all those
>> call sites by hand for the sake of "not introducing new code
>> with checkpatch problems". BTDT.
> 
> No you would not, don't be crazy.  Modfying a single line to rename a
> function from one thing to another could never be decreed as a "don't
> add bad code".  And if someone says it is, well, you just found someone
> who doesn't know what they are doing :)

Ok. I do hope you are right, and my extrapolation from past
experience was wrong.

> Please continue with this idea, it's sane, and valid.

Alright, will do. Patch will follow later today, SCJ. Let's see
what the checkpatch police will say.

Regards,
Tilman

- -- 
Tilman Schmidt                    E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.12 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAktNjCQACgkQQ3+did9BuFsmMwCfReQ/mHQUApkpRWJZIG16PmvI
vO4Anj/A4kSwddg+RZV2Uj5LVUNlsBD3
=LxhW
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Greg KH <gregkh@suse.de> writes:

>> One of our older controllers had a similar design error in that the two
>> functions would sometimes have to be reset together.  The way we look
>> for the second function is:
>> 
>> 	dev = pci_dev_get(dev1);
>> 	while ((dev = pci_get_device(vendor_id, device_id, dev))) {
>> 		if (dev->bus == dev1->bus &&
>> 		    dev->devfn == dev1->devfn + 1) {
>> 			dev2 = dev;
>> 			break;
>> 		}
>> 	}
>> 
>> I assume this is 'properly' since no-one has told me otherwise.
>
> As of right now, yes, this is the "correct" way to do this.
>
> I need to dig up the "let multiple drivers bind to a single device"
> patch set to make this not necessary in the future.

Ben and Greg, thanks.

Greg KH <gregkh@suse.de> writes:
>
> Close, but if you do this, please name the function
> hisax_find_pci_device() or something, and change the drivers to use it
> instead.  Also put a big fat warning in the function that calling this
> is unsafe for any PCI hotplug type machine.

It would be better to enforce this constraint at runtime.

e.g. set some global variable that forbids device removal
and warn in the kernel log. In theory this could be also
done per device, but I guess that would be more effort.

Of course it cannot prevent the user from really removing
a device, but at least the pci_dev could stay around.

-Andi

> e.g. set some global variable that forbids device removal
> and warn in the kernel log. In theory this could be also
> done per device, but I guess that would be more effort.

There is a simpler way to do that, which is to just leak a reference in
the hisax_find_pci_device hack. The pci_dev won't be going anywhere then.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Jan 14, 2010 at 11:02:14AM +0000, Alan Cox wrote:
> > e.g. set some global variable that forbids device removal
> > and warn in the kernel log. In theory this could be also
> > done per device, but I guess that would be more effort.
> 
> There is a simpler way to do that, which is to just leak a reference in
> the hisax_find_pci_device hack. The pci_dev won't be going anywhere then.

You just have to do it once, otherwise it'll fail after 4 billion times.

-Andi

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 2010-01-14 12:03 schrieb Andi Kleen:
> On Thu, Jan 14, 2010 at 11:02:14AM +0000, Alan Cox wrote:
[It would be better to enforce this constraint at runtime.]
>> There is a simpler way to do that, which is to just leak a reference in
>> the hisax_find_pci_device hack. The pci_dev won't be going anywhere then.
> 
> You just have to do it once, otherwise it'll fail after 4 billion times.

I guess we could live with that. pci_find_device() or its successor, the
hisax_find_pci_device() hack, is only called during device setup, once
for most drivers and at the very most 24 times for hfc_pci. Someone
would have to rmmod/insmod a HiSax module more than a hundred million
times to accumulate four billion calls.

- -- 
Tilman Schmidt                    E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.12 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAktPEHMACgkQQ3+did9BuFuQ/gCgkhu6RJIwrpIDAJMWbO1mZgOd
BKwAnAh6kX5F/+dFPIrK/XLovEAv/R7O
=fflE
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html