After the following patch to mark the isa region busy and applying a few
patches[1], I was able to kexec boot into an all-yes-config kernel from
linux-next, with the following KCONFIG_ALLCONFIG file:
# CONFIG_KALLSYMS_EXTRA_PASS is not set
# CONFIG_CRASH_DUMP is not set
CONFIG_PPC_EARLY_DEBUG_RTAS_CONSOLE=y
# CONFIG_NSC_FIR is not set
# CONFIG_SMC_IRCC_FIR is not set
# CONFIG_ALI_FIR is not set
# CONFIG_TCG_NSC is not set
# CONFIG_TCG_ATMEL is not set
# CONFIG_SENSORS_F71805F is not set
# CONFIG_SENSORS_F71882FG is not set
# CONFIG_SENSORS_F75375S is not set
# CONFIG_SENSORS_IT87 is not set
# CONFIG_SENSORS_PC87360 is not set
# CONFIG_SENSORS_PC87427 is not set
# CONFIG_SENSORS_DME1737 is not set
# CONFIG_SENSORS_SMSC47M1 is not set
# CONFIG_SENSORS_SMSC47B397 is not set
# CONFIG_SENSORS_VT1211 is not set
# CONFIG_SENSORS_W83627HF is not set
# CONFIG_SENSORS_W83627EHF is not set
While the first two might not be required, and the third is just
selecting the right platform, it would be nice to get the drivers
to play as well as the rest of the kernel.
The drivers all are for super-io chips, either irda/FIR drivers or hwmon,
and poke at isa ports without checking request_region.
It would be great if these 17 super-io drivers would check for resource
availability before poking at io ports.
[not that I expect this to merge as it, but maybe insert-resource or something]
[1] 3 section layout patches and the MTD DISKONCHIP patch,
see linuxppc-dev for full list
diff --git a/arch/powerpc/platforms/pseries/pci.c b/arch/powerpc/platforms/pseries/pci.c
index 2c6ded2..db54620 100644
--- a/arch/powerpc/platforms/pseries/pci.c
+++ b/arch/powerpc/platforms/pseries/pci.c
@@ -58,8 +58,10 @@ DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, pcibios_name_device);
static void __init pSeries_request_regions(void)
{
- if (!isa_io_base)
+ if (!isa_io_base) {
+ request_region(0x0,0x400,"isa");
return;
+ }
request_region(0x20,0x20,"pic1");
request_region(0xa0,0x20,"pic2");
Such a hardcoded address can cause a checkstop or machine check if
the driver is in the kernel but the address is not acknowledged.
Both drivers allow an address to be specified as either a module
parameter or config option. Any future powerpc board should either
use one of these methods or find the address in the device tree.
Signed-off-by: Milton Miller <[email protected]>
---
There is no powerpc defconfig that sets either CONFIG_DOC200{0,1*} nor
CONFIG_MTD_NAND_DISKONCHIP in next-20080710.
diff --git a/drivers/mtd/devices/docprobe.c b/drivers/mtd/devices/docprobe.c
index 6e5d811..6e62922 100644
--- a/drivers/mtd/devices/docprobe.c
+++ b/drivers/mtd/devices/docprobe.c
@@ -76,8 +76,6 @@ static unsigned long __initdata doc_locations[] = {
0xe0000, 0xe2000, 0xe4000, 0xe6000,
0xe8000, 0xea000, 0xec000, 0xee000,
#endif /* CONFIG_MTD_DOCPROBE_HIGH */
-#elif defined(__PPC__)
- 0xe4000000,
#else
#warning Unknown architecture for DiskOnChip. No default probe locations defined
#endif
diff --git a/drivers/mtd/nand/diskonchip.c b/drivers/mtd/nand/diskonchip.c
index cd4393c..765d4f0 100644
--- a/drivers/mtd/nand/diskonchip.c
+++ b/drivers/mtd/nand/diskonchip.c
@@ -52,8 +52,6 @@ static unsigned long __initdata doc_locations[] = {
0xe0000, 0xe2000, 0xe4000, 0xe6000,
0xe8000, 0xea000, 0xec000, 0xee000,
#endif /* CONFIG_MTD_DOCPROBE_HIGH */
-#elif defined(__PPC__)
- 0xe4000000,
#else
#warning Unknown architecture for DiskOnChip. No default probe locations defined
#endif
Milton Miller wrote:
> After the following patch to mark the isa region busy and applying a few
> patches[1], I was able to kexec boot into an all-yes-config kernel from
> linux-next, with the following KCONFIG_ALLCONFIG file:
>
> # CONFIG_KALLSYMS_EXTRA_PASS is not set
> # CONFIG_CRASH_DUMP is not set
> CONFIG_PPC_EARLY_DEBUG_RTAS_CONSOLE=y
> # CONFIG_NSC_FIR is not set
> # CONFIG_SMC_IRCC_FIR is not set
> # CONFIG_ALI_FIR is not set
> # CONFIG_TCG_NSC is not set
> # CONFIG_TCG_ATMEL is not set
> # CONFIG_SENSORS_F71805F is not set
> # CONFIG_SENSORS_F71882FG is not set
> # CONFIG_SENSORS_F75375S is not set
> # CONFIG_SENSORS_IT87 is not set
> # CONFIG_SENSORS_PC87360 is not set
> # CONFIG_SENSORS_PC87427 is not set
> # CONFIG_SENSORS_DME1737 is not set
> # CONFIG_SENSORS_SMSC47M1 is not set
> # CONFIG_SENSORS_SMSC47B397 is not set
> # CONFIG_SENSORS_VT1211 is not set
> # CONFIG_SENSORS_W83627HF is not set
> # CONFIG_SENSORS_W83627EHF is not set
>
> While the first two might not be required, and the third is just
> selecting the right platform, it would be nice to get the drivers
> to play as well as the rest of the kernel.
>
> The drivers all are for super-io chips, either irda/FIR drivers or hwmon,
> and poke at isa ports without checking request_region.
>
Erm,
The superio sensor drivers only poke the superio chip registers without request
region during the probe phase, iow they try to detect the chip, using a widely
document and standardized (part of isa pnp AFAIK) procedure on standardized ports.
Let me try to explain a bit about superio chips, they have 2 superio control
registers (an index and data register) with which things like a manufacturer
and device id can be read, besides these id registers they also have a set of
registers with config for different logical devices. Once the id is matched,
the driver knows which logical device config to read, reads a (different) isa
base address + range from the logical device config, and then does a
request_region on the region actually used by the logical device.
The superio control registers are thus a sort of pci configuration space if you
want, doing a request_region on these is not such a good idea, as multiple
drivers (for different logical devices within the superio device) may use
these, so trying to gain exclusive access will lead to troubles.
I hope with this info about the problem space, that you maybe have a suggestion
on howto fix this?
Regards,
Hans
>
On Jul 10, 2008, at 4:33 PM, Hans de Goede wrote:
> Milton Miller wrote:
>> After the following patch to mark the isa region busy and applying a
>> few
>> patches[1], I was able to kexec boot into an all-yes-config kernel
>> from linux-next, with the following KCONFIG_ALLCONFIG file:
...
>> While the first two might not be required, and the third is just
>> selecting the right platform, it would be nice to get the drivers
>> to play as well as the rest of the kernel.
>> The drivers all are for super-io chips, either irda/FIR drivers or
>> hwmon,
>> and poke at isa ports without checking request_region.
>
> Erm,
>
> The superio sensor drivers only poke the superio chip registers
> without request region during the probe phase, iow they try to detect
> the chip, using a widely document and standardized (part of isa pnp
> AFAIK) procedure on standardized ports.
>
> Let me try to explain a bit about superio chips, they have 2 superio
> control registers (an index and data register) with which things like
> a manufacturer and device id can be read, besides these id registers
> they also have a set of registers with config for different logical
> devices. Once the id is matched, the driver knows which logical device
> config to read, reads a (different) isa base address + range from the
> logical device config, and then does a request_region on the region
> actually used by the logical device.
>
> The superio control registers are thus a sort of pci configuration
> space if you want, doing a request_region on these is not such a good
> idea, as multiple drivers (for different logical devices within the
> superio device) may use these, so trying to gain exclusive access will
> lead to troubles.
>
> I hope with this info about the problem space, that you maybe have a
> suggestion on howto fix this?
>
My point is that they are probing without even knowing that a such a IO
range exists.
These are the only drivers left in the tree that still do this. (At
least that are not
blocked on a powerpc allyesconfig for 64-bit, !CONFIG_VIRT_TO_BUS).
One could make a superio driver, and create sub-devices for the IR,
I2C, floppy, parallel, etc
nodes.
I would even accept a check_region (horrors!), it would impove the
situation.
But most other drivers do this by request_region, probe, then release
the region afterwards.
Besides, one could argue the superio region should be requested while
probing, to prevent other cpus from poking at the super io chip at the
same time. Think of it as missing locking.
milton
Hi Hans, hi Milton,
On Thu, 10 Jul 2008 16:51:33 -0500, Milton Miller wrote:
>
> On Jul 10, 2008, at 4:33 PM, Hans de Goede wrote:
>
> > Milton Miller wrote:
> >> After the following patch to mark the isa region busy and applying a
> >> few
> >> patches[1], I was able to kexec boot into an all-yes-config kernel
> >> from linux-next, with the following KCONFIG_ALLCONFIG file:
> ...
> >> While the first two might not be required, and the third is just
> >> selecting the right platform, it would be nice to get the drivers
> >> to play as well as the rest of the kernel.
> >> The drivers all are for super-io chips, either irda/FIR drivers or
> >> hwmon,
> >> and poke at isa ports without checking request_region.
> >
> > Erm,
> >
> > The superio sensor drivers only poke the superio chip registers
> > without request region during the probe phase, iow they try to detect
> > the chip, using a widely document and standardized (part of isa pnp
> > AFAIK) procedure on standardized ports.
> >
> > Let me try to explain a bit about superio chips, they have 2 superio
> > control registers (an index and data register) with which things like
> > a manufacturer and device id can be read, besides these id registers
> > they also have a set of registers with config for different logical
> > devices. Once the id is matched, the driver knows which logical device
> > config to read, reads a (different) isa base address + range from the
> > logical device config, and then does a request_region on the region
> > actually used by the logical device.
> >
> > The superio control registers are thus a sort of pci configuration
> > space if you want, doing a request_region on these is not such a good
> > idea, as multiple drivers (for different logical devices within the
> > superio device) may use these, so trying to gain exclusive access will
> > lead to troubles.
> >
> > I hope with this info about the problem space, that you maybe have a
> > suggestion on howto fix this?
>
> My point is that they are probing without even knowing that a such a IO
> range exists.
>
> These are the only drivers left in the tree that still do this. (At
> least that are not
> blocked on a powerpc allyesconfig for 64-bit, !CONFIG_VIRT_TO_BUS).
I guess we could make all these drivers depend on X86, I suspect that
these chips are only used in PCs?
> One could make a superio driver, and create sub-devices for the IR,
> I2C, floppy, parallel, etc
> nodes.
There have been proposals to do this, and this would indeed be a very
good idea, but unfortunately nobody took the time to implement this
properly, push it upstream and volunteer to maintain it. The problem is
that you don't need just a "driver", but a new subsystem, that needs to
be designed and maintained.
> I would even accept a check_region (horrors!), it would impove the
> situation.
>
> But most other drivers do this by request_region, probe, then release
> the region afterwards.
I agree that the hwmon drivers should do this, as long as no superio
subsystem exist. This should solve the problem at hand and might even
help spot bad drivers which use the configuration space for longer than
initialization time.
> Besides, one could argue the superio region should be requested while
> probing, to prevent other cpus from poking at the super io chip at the
> same time. Think of it as missing locking.
Agreed.
Milton, care to submit a patch fixing the affected hwmon drivers?
Thanks,
--
Jean Delvare
Jean Delvare wrote:
> Hi Hans, hi Milton,
>
<snip>
>> One could make a superio driver, and create sub-devices for the IR,
>> I2C, floppy, parallel, etc
>> nodes.
>
> There have been proposals to do this, and this would indeed be a very
> good idea, but unfortunately nobody took the time to implement this
> properly, push it upstream and volunteer to maintain it. The problem is
> that you don't need just a "driver", but a new subsystem, that needs to
> be designed and maintained.
>
Well, I believe there have been some lightweight superio locking coordinator
patches been floating around on the lm_sensors list, and I have reviewed them
and then a new version was done with my issues fixed.
I kinda liked the proposed solution there, it was quite simple, moved all the
generic superio stuff into generic superio code, and added locking for super io
access from multiple drivers, what ever happened to those patches?
If were to start using those, we could actually do a request region and then
never release it, as things should be.
Regards,
Hans
On Fri, 11 Jul 2008 09:27:26 +0200, Hans de Goede wrote:
> Jean Delvare wrote:
> > Hi Hans, hi Milton,
> >
>
> <snip>
>
> >> One could make a superio driver, and create sub-devices for the IR,
> >> I2C, floppy, parallel, etc
> >> nodes.
> >
> > There have been proposals to do this, and this would indeed be a very
> > good idea, but unfortunately nobody took the time to implement this
> > properly, push it upstream and volunteer to maintain it. The problem is
> > that you don't need just a "driver", but a new subsystem, that needs to
> > be designed and maintained.
>
> Well, I believe there have been some lightweight superio locking coordinator
> patches been floating around on the lm_sensors list, and I have reviewed them
> and then a new version was done with my issues fixed.
>
> I kinda liked the proposed solution there, it was quite simple, moved all the
> generic superio stuff into generic superio code, and added locking for super io
> access from multiple drivers, what ever happened to those patches?
As far as I know, nothing, and this is the problem. Somebody needs to
step up and call him/herself the maintainer of the new code, and push
it upstream and convert all the drivers (hwmon, watchdog, parallel
port...) to make use of it. And I am not the one to do this, I am busy
enough as is with i2c and hwmon.
> If were to start using those, we could actually do a request region and then
> never release it, as things should be.
Yes, if we have a superio access coordinator, it can request the region
and not release it. But as long as we don't have that, I agree with
Milton that the individual drivers should temporarily request the
Super-I/O region before accessing it.
--
Jean Delvare
Jean Delvare wrote:
> On Fri, 11 Jul 2008 09:27:26 +0200, Hans de Goede wrote:
>> Jean Delvare wrote:
>>> Hi Hans, hi Milton,
>>>
>> <snip>
>>
>>>> One could make a superio driver, and create sub-devices for the IR,
>>>> I2C, floppy, parallel, etc
>>>> nodes.
>>> There have been proposals to do this, and this would indeed be a very
>>> good idea, but unfortunately nobody took the time to implement this
>>> properly, push it upstream and volunteer to maintain it. The problem is
>>> that you don't need just a "driver", but a new subsystem, that needs to
>>> be designed and maintained.
>> Well, I believe there have been some lightweight superio locking coordinator
>> patches been floating around on the lm_sensors list, and I have reviewed them
>> and then a new version was done with my issues fixed.
>>
>> I kinda liked the proposed solution there, it was quite simple, moved all the
>> generic superio stuff into generic superio code, and added locking for super io
>> access from multiple drivers, what ever happened to those patches?
>
> As far as I know, nothing, and this is the problem. Somebody needs to
> step up and call him/herself the maintainer of the new code, and push
> it upstream and convert all the drivers (hwmon, watchdog, parallel
> port...) to make use of it. And I am not the one to do this, I am busy
> enough as is with i2c and hwmon.
>
Ok, I've mailed Jim Cromie, the author of the superio access patches
from end 2007 and told him we're still interested in this and asked him
if he is willing to drive this forward.
Regards,
Hans
Hi Hans, Milton, Jean,
On Sun, Jul 13, 2008 at 12:31 AM, Hans de Goede <[email protected]> wrote:
> Jean Delvare wrote:
>> On Fri, 11 Jul 2008 09:27:26 +0200, Hans de Goede wrote:
>>> Jean Delvare wrote:
>>>> Hi Hans, hi Milton,
>>>>
>>> <snip>
>>>
>>>>> One could make a superio driver, and create sub-devices for the IR,
>>>>> I2C, floppy, parallel, etc
>>>>> nodes.
>>>> There have been proposals to do this, and this would indeed be a very
>>>> good idea, but unfortunately nobody took the time to implement this
>>>> properly, push it upstream and volunteer to maintain it. The problem is
>>>> that you don't need just a "driver", but a new subsystem, that needs to
>>>> be designed and maintained.
>>> Well, I believe there have been some lightweight superio locking coordinator
>>> patches been floating around on the lm_sensors list, and I have reviewed them
>>> and then a new version was done with my issues fixed.
>>>
>>> I kinda liked the proposed solution there, it was quite simple, moved all the
>>> generic superio stuff into generic superio code, and added locking for super io
>>> access from multiple drivers, what ever happened to those patches?
>>
>> As far as I know, nothing, and this is the problem. Somebody needs to
>> step up and call him/herself the maintainer of the new code, and push
>> it upstream and convert all the drivers (hwmon, watchdog, parallel
>> port...) to make use of it. And I am not the one to do this, I am busy
>> enough as is with i2c and hwmon.
>>
>
> Ok, I've mailed Jim Cromie, the author of the superio access patches
> from end 2007 and told him we're still interested in this and asked him
> if he is willing to drive this forward.
I propose writing a subsystem driver. (Is that properly called "The
SuperIO Bus Driver"?) If no one thinks it's a really bad idea I will
put together some code and submit it for review, and maintain it.
Some hwmon chips have odd / unique probe sequences. IMHO this is where
the design needs to be inspected. One of those is the w83627ehf, which
Jean and Hans are familiar enough with to check my work.
Thoughts?
David
David Hubbard wrote:
> Hi Hans, Milton, Jean,
>
> On Sun, Jul 13, 2008 at 12:31 AM, Hans de Goede <[email protected]> wrote:
>> Jean Delvare wrote:
>>> On Fri, 11 Jul 2008 09:27:26 +0200, Hans de Goede wrote:
>>>> Jean Delvare wrote:
>>>>> Hi Hans, hi Milton,
>>>>>
>>>> <snip>
>>>>
>>>>>> One could make a superio driver, and create sub-devices for the IR,
>>>>>> I2C, floppy, parallel, etc
>>>>>> nodes.
>>>>> There have been proposals to do this, and this would indeed be a very
>>>>> good idea, but unfortunately nobody took the time to implement this
>>>>> properly, push it upstream and volunteer to maintain it. The problem is
>>>>> that you don't need just a "driver", but a new subsystem, that needs to
>>>>> be designed and maintained.
>>>> Well, I believe there have been some lightweight superio locking coordinator
>>>> patches been floating around on the lm_sensors list, and I have reviewed them
>>>> and then a new version was done with my issues fixed.
>>>>
>>>> I kinda liked the proposed solution there, it was quite simple, moved all the
>>>> generic superio stuff into generic superio code, and added locking for super io
>>>> access from multiple drivers, what ever happened to those patches?
>>> As far as I know, nothing, and this is the problem. Somebody needs to
>>> step up and call him/herself the maintainer of the new code, and push
>>> it upstream and convert all the drivers (hwmon, watchdog, parallel
>>> port...) to make use of it. And I am not the one to do this, I am busy
>>> enough as is with i2c and hwmon.
>>>
>> Ok, I've mailed Jim Cromie, the author of the superio access patches
>> from end 2007 and told him we're still interested in this and asked him
>> if he is willing to drive this forward.
>
> I propose writing a subsystem driver. (Is that properly called "The
> SuperIO Bus Driver"?) If no one thinks it's a really bad idea I will
> put together some code and submit it for review, and maintain it.
>
> Some hwmon chips have odd / unique probe sequences. IMHO this is where
> the design needs to be inspected. One of those is the w83627ehf, which
> Jean and Hans are familiar enough with to check my work.
>
> Thoughts?
I'm afraid that making this a "bus" will be a bit overkill. Jim's patches are
quite simple and seem todo the job.
Also keep in mind that most users will be platform devices which just want to
use the superio registers to find out the baseaddress of their logical device,
a whole bus seems overkill for this, would the hwmon driver then need to be a
superio_driver (as well as an platform_driver) or can the bus be queried / used
without having to be a bustype-driver?
Regards,
Hans
Hi Hans,
>> I propose writing a subsystem driver. (Is that properly called "The
>> SuperIO Bus Driver"?) If no one thinks it's a really bad idea I will
>> put together some code and submit it for review, and maintain it.
>>
>> Some hwmon chips have odd / unique probe sequences. IMHO this is where
>> the design needs to be inspected. One of those is the w83627ehf, which
>> Jean and Hans are familiar enough with to check my work.
>>
>> Thoughts?
>
> I'm afraid that making this a "bus" will be a bit overkill. Jim's patches
> are quite simple and seem todo the job.
>
> Also keep in mind that most users will be platform devices which just want
> to use the superio registers to find out the baseaddress of their logical
> device, a whole bus seems overkill for this, would the hwmon driver then
> need to be a superio_driver (as well as an platform_driver) or can the bus
> be queried / used
> without having to be a bustype-driver?
I think that's a valid point. I am willing to keep it small, but I
would prefer to follow the pattern set in other subsystems. It may be
my lack of experience in designing a subsystem showing here! What are
some alternative ways to implement it?
In other words, Jim's patches are a good start, but maybe I am
misunderstanding them. I see it as the superio-locks module, a driver
that other drivers would depend on / auto-load. Is that something
other subsystems also do?
Regards,
David
On Sun, 13 Jul 2008 15:26:56 -0600, David Hubbard wrote:
> Hi Hans,
>
> >> I propose writing a subsystem driver. (Is that properly called "The
> >> SuperIO Bus Driver"?) If no one thinks it's a really bad idea I will
> >> put together some code and submit it for review, and maintain it.
> >>
> >> Some hwmon chips have odd / unique probe sequences. IMHO this is where
> >> the design needs to be inspected. One of those is the w83627ehf, which
> >> Jean and Hans are familiar enough with to check my work.
> >>
> >> Thoughts?
> >
> > I'm afraid that making this a "bus" will be a bit overkill. Jim's patches
> > are quite simple and seem todo the job.
> >
> > Also keep in mind that most users will be platform devices which just want
> > to use the superio registers to find out the baseaddress of their logical
> > device, a whole bus seems overkill for this, would the hwmon driver then
> > need to be a superio_driver (as well as an platform_driver) or can the bus
> > be queried / used
> > without having to be a bustype-driver?
>
> I think that's a valid point. I am willing to keep it small, but I
> would prefer to follow the pattern set in other subsystems. It may be
> my lack of experience in designing a subsystem showing here! What are
> some alternative ways to implement it?
>
> In other words, Jim's patches are a good start, but maybe I am
> misunderstanding them. I see it as the superio-locks module, a driver
> that other drivers would depend on / auto-load. Is that something
> other subsystems also do?
Well, there are two approaches to the problem. The first approach
(which I think Jim took in his patches? I don't really remember) is to
simply solve the problem of concurrent I/O access to the Super-I/O
configuration ports (typically 0x2e/0x2f or 0x4e/0x4f). That would be a
simple driver requesting the ports in question and exporting an API for
other drivers to access them in a safe way.
The second approach is to make it a whole subsystem, as David is
suggesting. The Super-I/O driver would then not only request the I/O
ports, it would also identify which Super-I/O is there, and it would
create devices (in the Linux device driver model sense of the term) for
every logical device we are interested in (amongst which the hwmon
ones.) The hwmon drivers would have to be converted from platform
drivers to superio drivers.
Each approach has its advantages. The first one is rather simple and
also very generic in nature. It could be reused for other purposes. The
second one offers automatic loading of hwmon drivers, which would be
nice to have.
There's probably a middle way which would keep the simplicity of the
first approach while still allowing for driver auto-loading, without
changing the bus type of all drivers. It would probably take some
research though.
Me, I don't really care which path we choose, given that I am not the
one who will take care of it. All I have to say is that this has been
discussed several times over the past 2 years and nothing came out of
it (as far as the mainline kernel is concerned, at least) so whatever
is done now, what really matters is that it makes it into the kernel
quickly. We have some drivers missing features because of this (for
example real-time reading of VID pin values.)
This should also not prevent us from fixing the drivers now so that
they temporarily request the Super-I/O ports they are using, as Milton
suggested. Not only we don't know when we will have something better to
offer, but it might also help us find conflicts between drivers, so
that we know which drivers should make use of the future superio
driver. This could also reveal conflicts with PNP BIOS reservations,
etc. Milton, will you write a patch?
--
Jean Delvare
On Jul 14, 2008, at 2:59 AM, Jean Delvare wrote:
> On Sun, 13 Jul 2008 15:26:56 -0600, David Hubbard wrote:
>> Hi Hans,
>>
>>>> I propose writing a subsystem driver. (Is that properly called "The
>>>> SuperIO Bus Driver"?) If no one thinks it's a really bad idea I will
>>>> put together some code and submit it for review, and maintain it.
>>>>
>>>> Some hwmon chips have odd / unique probe sequences. IMHO this is
>>>> where
>>>> the design needs to be inspected. One of those is the w83627ehf,
>>>> which
>>>> Jean and Hans are familiar enough with to check my work.
>>>>
>>>> Thoughts?
>>>
>>> I'm afraid that making this a "bus" will be a bit overkill. Jim's
>>> patches
>>> are quite simple and seem todo the job.
>>>
>>> Also keep in mind that most users will be platform devices which
>>> just want
>>> to use the superio registers to find out the baseaddress of their
>>> logical
>>> device, a whole bus seems overkill for this, would the hwmon driver
>>> then
>>> need to be a superio_driver (as well as an platform_driver) or can
>>> the bus
>>> be queried / used
>>> without having to be a bustype-driver?
>>
>> I think that's a valid point. I am willing to keep it small, but I
>> would prefer to follow the pattern set in other subsystems. It may be
>> my lack of experience in designing a subsystem showing here! What are
>> some alternative ways to implement it?
>>
>> In other words, Jim's patches are a good start, but maybe I am
>> misunderstanding them. I see it as the superio-locks module, a driver
>> that other drivers would depend on / auto-load. Is that something
>> other subsystems also do?
>
> Well, there are two approaches to the problem. The first approach
> (which I think Jim took in his patches? I don't really remember) is to
> simply solve the problem of concurrent I/O access to the Super-I/O
> configuration ports (typically 0x2e/0x2f or 0x4e/0x4f). That would be a
> simple driver requesting the ports in question and exporting an API for
> other drivers to access them in a safe way.
>
> The second approach is to make it a whole subsystem, as David is
> suggesting. The Super-I/O driver would then not only request the I/O
> ports, it would also identify which Super-I/O is there, and it would
> create devices (in the Linux device driver model sense of the term) for
> every logical device we are interested in (amongst which the hwmon
> ones.) The hwmon drivers would have to be converted from platform
> drivers to superio drivers.
>
> Each approach has its advantages. The first one is rather simple and
> also very generic in nature. It could be reused for other purposes. The
> second one offers automatic loading of hwmon drivers, which would be
> nice to have.
>
> There's probably a middle way which would keep the simplicity of the
> first approach while still allowing for driver auto-loading, without
> changing the bus type of all drivers. It would probably take some
> research though.
I haven't done the research, but it might be keep superio as
a platform driver, and keep the clients as platform drivers. Only
have the superio driver probe and discover the subcomponent
addresses and then create the platform devices as children
instead of having each driver create its own platform device.
(This all assumes they are all platform devices in sysfs, I have
not looked).
This is all because in the platform bus the bus driver does not
discover the addresses but relies on drivers or platform setup code.
> Me, I don't really care which path we choose, given that I am not the
> one who will take care of it. All I have to say is that this has been
> discussed several times over the past 2 years and nothing came out of
> it (as far as the mainline kernel is concerned, at least) so whatever
> is done now, what really matters is that it makes it into the kernel
> quickly. We have some drivers missing features because of this (for
> example real-time reading of VID pin values.)
>
> This should also not prevent us from fixing the drivers now so that
> they temporarily request the Super-I/O ports they are using, as Milton
> suggested. Not only we don't know when we will have something better to
> offer, but it might also help us find conflicts between drivers, so
> that we know which drivers should make use of the future superio
> driver. This could also reveal conflicts with PNP BIOS reservations,
> etc. Milton, will you write a patch?
Well, that is the second time you asked me, so I guess I should respond.
While I it is possible for me to write this patch, my schedule and
priority list predict it would not be before the merge window closes.
In
fact, I'm not sure when it would come out. It might be argued it could
go in early -rc, but I would fear somebody's chip will not be detected
with the additional check. For example, the port may reserved as mother
board resources or something. Also, I have no hardware to test any
proposed patch, so it would be compile check only.
milton
Milton Miller wrote:
> On Jul 14, 2008, at 2:59 AM, Jean Delvare wrote:
>> On Sun, 13 Jul 2008 15:26:56 -0600, David Hubbard wrote:
>>> Hi Hans,
>>>
>>>>> I propose writing a subsystem driver. (Is that properly called "The
>>>>> SuperIO Bus Driver"?) If no one thinks it's a really bad idea I will
>>>>> put together some code and submit it for review, and maintain it.
>>>>>
>>>>> Some hwmon chips have odd / unique probe sequences. IMHO this is
>>>>> where
>>>>> the design needs to be inspected. One of those is the w83627ehf,
>>>>> which
>>>>> Jean and Hans are familiar enough with to check my work.
>>>>>
>>>>> Thoughts?
>>>> I'm afraid that making this a "bus" will be a bit overkill. Jim's
>>>> patches
>>>> are quite simple and seem todo the job.
>>>>
>>>> Also keep in mind that most users will be platform devices which
>>>> just want
>>>> to use the superio registers to find out the baseaddress of their
>>>> logical
>>>> device, a whole bus seems overkill for this, would the hwmon driver
>>>> then
>>>> need to be a superio_driver (as well as an platform_driver) or can
>>>> the bus
>>>> be queried / used
>>>> without having to be a bustype-driver?
>>> I think that's a valid point. I am willing to keep it small, but I
>>> would prefer to follow the pattern set in other subsystems. It may be
>>> my lack of experience in designing a subsystem showing here! What are
>>> some alternative ways to implement it?
>>>
>>> In other words, Jim's patches are a good start, but maybe I am
>>> misunderstanding them. I see it as the superio-locks module, a driver
>>> that other drivers would depend on / auto-load. Is that something
>>> other subsystems also do?
>> Well, there are two approaches to the problem. The first approach
>> (which I think Jim took in his patches? I don't really remember) is to
>> simply solve the problem of concurrent I/O access to the Super-I/O
>> configuration ports (typically 0x2e/0x2f or 0x4e/0x4f). That would be a
>> simple driver requesting the ports in question and exporting an API for
>> other drivers to access them in a safe way.
>>
>> The second approach is to make it a whole subsystem, as David is
>> suggesting. The Super-I/O driver would then not only request the I/O
>> ports, it would also identify which Super-I/O is there, and it would
>> create devices (in the Linux device driver model sense of the term) for
>> every logical device we are interested in (amongst which the hwmon
>> ones.) The hwmon drivers would have to be converted from platform
>> drivers to superio drivers.
>>
>> Each approach has its advantages. The first one is rather simple and
>> also very generic in nature. It could be reused for other purposes. The
>> second one offers automatic loading of hwmon drivers, which would be
>> nice to have.
>>
>> There's probably a middle way which would keep the simplicity of the
>> first approach while still allowing for driver auto-loading, without
>> changing the bus type of all drivers. It would probably take some
>> research though.
>
> I haven't done the research, but it might be keep superio as
> a platform driver, and keep the clients as platform drivers. Only
> have the superio driver probe and discover the subcomponent
> addresses and then create the platform devices as children
> instead of having each driver create its own platform device.
> (This all assumes they are all platform devices in sysfs, I have
> not looked).
>
> This is all because in the platform bus the bus driver does not
> discover the addresses but relies on drivers or platform setup code.
>
This sounds like a good plan, rather then add a new bus type add a superio
platform driver which does superio probing and registering of platform devices
for discovered logical devices.
This superio platform driver then needs to also export some functions of those
few logical devices which need access to the superio registers for more then
just finding out their own base address.
I guess that it then would be best to load this superio driver by default on
most systems.
How does this all mix and match with isapnp, it feels to me we're doing
somewhat the same as isapnp here.
Regards,
Hans
Hi Hans,
On Mon, Jul 14, 2008 at 11:30 AM, Hans de Goede <[email protected]> wrote:
> Milton Miller wrote:
>> I haven't done the research, but it might be keep superio as
>> a platform driver, and keep the clients as platform drivers. Only
>> have the superio driver probe and discover the subcomponent
>> addresses and then create the platform devices as children
>> instead of having each driver create its own platform device.
>> (This all assumes they are all platform devices in sysfs, I have
>> not looked).
>>
>> This is all because in the platform bus the bus driver does not
>> discover the addresses but relies on drivers or platform setup code.
>>
>
> This sounds like a good plan, rather then add a new bus type add a superio
> platform driver which does superio probing and registering of platform devices
> for discovered logical devices.
>
> This superio platform driver then needs to also export some functions of those
> few logical devices which need access to the superio registers for more then
> just finding out their own base address.
>
> I guess that it then would be best to load this superio driver by default on
> most systems.
>
> How does this all mix and match with isapnp, it feels to me we're doing
> somewhat the same as isapnp here.
Is there any way to use lspci and start at the LPC bridge, then find
the SuperIO chip's IO address? What about ACPI tables? Perhaps probing
logic could look for an LPC bridge before probing certain IO addresses
even if the addresses are not in the LPC bridge config.
A superio platform driver is a good way to go -- it fits with the way
the platform bus does things. Also, Jim's patches are almost there
already.
Thanks,
David
Hi Milton,
On Mon, 14 Jul 2008 12:09:03 -0500, Milton Miller wrote:
> On Jul 14, 2008, at 2:59 AM, Jean Delvare wrote:
> > Well, there are two approaches to the problem. The first approach
> > (which I think Jim took in his patches? I don't really remember) is to
> > simply solve the problem of concurrent I/O access to the Super-I/O
> > configuration ports (typically 0x2e/0x2f or 0x4e/0x4f). That would be a
> > simple driver requesting the ports in question and exporting an API for
> > other drivers to access them in a safe way.
> >
> > The second approach is to make it a whole subsystem, as David is
> > suggesting. The Super-I/O driver would then not only request the I/O
> > ports, it would also identify which Super-I/O is there, and it would
> > create devices (in the Linux device driver model sense of the term) for
> > every logical device we are interested in (amongst which the hwmon
> > ones.) The hwmon drivers would have to be converted from platform
> > drivers to superio drivers.
> >
> > Each approach has its advantages. The first one is rather simple and
> > also very generic in nature. It could be reused for other purposes. The
> > second one offers automatic loading of hwmon drivers, which would be
> > nice to have.
> >
> > There's probably a middle way which would keep the simplicity of the
> > first approach while still allowing for driver auto-loading, without
> > changing the bus type of all drivers. It would probably take some
> > research though.
>
> I haven't done the research, but it might be keep superio as
> a platform driver, and keep the clients as platform drivers. Only
> have the superio driver probe and discover the subcomponent
> addresses and then create the platform devices as children
> instead of having each driver create its own platform device.
> (This all assumes they are all platform devices in sysfs, I have
> not looked).
>
> This is all because in the platform bus the bus driver does not
> discover the addresses but relies on drivers or platform setup code.
That's more or less what I had in mind, yes.
> > (...) Milton, will you write a patch?
>
> Well, that is the second time you asked me, so I guess I should respond.
>
> While I it is possible for me to write this patch, my schedule and
> priority list predict it would not be before the merge window closes.
> In fact, I'm not sure when it would come out. It might be argued it
> could go in early -rc, but I would fear somebody's chip will not be detected
> with the additional check. For example, the port may reserved as mother
> board resources or something.
I really don't see this as something for kernel 2.6.27, it's too late
already. It doesn't fix any actual problem anyway (none that can be
fixed by not loading drivers you don't need, at least.) That would be
for 2.6.28, so we have plenty of time to test the changes and ensure
they do not break anything. As you are the one who reported the issue
as something that was bothering you personally, I expected you to also
spend some time trying to address it.
> (...) Also, I have no hardware to test any
> proposed patch, so it would be compile check only.
If you write the patches and post them to the lm-sensors list, I am
certain that you will find several volunteers for testing. Me, I can
offer testing of the it87, f71805f and w83627ehf drivers.
Thanks,
--
Jean Delvare
On Mon, 14 Jul 2008 11:55:01 -0600, David Hubbard wrote:
> Hi Hans,
>
> On Mon, Jul 14, 2008 at 11:30 AM, Hans de Goede <[email protected]> wrote:
> > Milton Miller wrote:
> >> I haven't done the research, but it might be keep superio as
> >> a platform driver, and keep the clients as platform drivers. Only
> >> have the superio driver probe and discover the subcomponent
> >> addresses and then create the platform devices as children
> >> instead of having each driver create its own platform device.
> >> (This all assumes they are all platform devices in sysfs, I have
> >> not looked).
> >>
> >> This is all because in the platform bus the bus driver does not
> >> discover the addresses but relies on drivers or platform setup code.
> >
> > This sounds like a good plan, rather then add a new bus type add a superio
> > platform driver which does superio probing and registering of platform devices
> > for discovered logical devices.
> >
> > This superio platform driver then needs to also export some functions of those
> > few logical devices which need access to the superio registers for more then
> > just finding out their own base address.
> >
> > I guess that it then would be best to load this superio driver by default on
> > most systems.
> >
> > How does this all mix and match with isapnp, it feels to me we're doing
> > somewhat the same as isapnp here.
>
> Is there any way to use lspci and start at the LPC bridge, then find
> the SuperIO chip's IO address? What about ACPI tables? Perhaps probing
> logic could look for an LPC bridge before probing certain IO addresses
> even if the addresses are not in the LPC bridge config.
I always assumed that there was no way to know in advance if a
Super-I/O (LPC) chip was present or not, let alone the exact model of
the chip. The I/O addresses are decoded by the Super-I/O chip itself,
and in general it has no relation to PCI. And I've never seen ports
0x2e/0x2f nor 0x4e/0x4f listed in /proc/ioports.
But of course if there is a way to know, we should use it. Avoiding
random access to I/O ports, even if they are relatively standard in
this case, is always good.
> A superio platform driver is a good way to go -- it fits with the way
> the platform bus does things. Also, Jim's patches are almost there
> already.
Good.
--
Jean Delvare
Hi Jean,
On Tue, Jul 15, 2008 at 2:36 AM, Jean Delvare <[email protected]> wrote:
>> Is there any way to use lspci and start at the LPC bridge, then find
>> the SuperIO chip's IO address? What about ACPI tables? Perhaps probing
>> logic could look for an LPC bridge before probing certain IO addresses
>> even if the addresses are not in the LPC bridge config.
>
> I always assumed that there was no way to know in advance if a
> Super-I/O (LPC) chip was present or not, let alone the exact model of
> the chip. The I/O addresses are decoded by the Super-I/O chip itself,
> and in general it has no relation to PCI. And I've never seen ports
> 0x2e/0x2f nor 0x4e/0x4f listed in /proc/ioports.
>
> But of course if there is a way to know, we should use it. Avoiding
> random access to I/O ports, even if they are relatively standard in
> this case, is always good.
I looked at my lspci output and did a little researching, and I think
the only thing we can deduce is that there is an LPC bridge, so
looking for a SuperIO is a good idea. If there is no LPC bridge
listed, I can't say whether probing the ports is a good idea or not.
David
Hi David,
On Tue, 15 Jul 2008 09:31:29 -0600, David Hubbard wrote:
> On Tue, Jul 15, 2008 at 2:36 AM, Jean Delvare <[email protected]> wrote:
> > I always assumed that there was no way to know in advance if a
> > Super-I/O (LPC) chip was present or not, let alone the exact model of
> > the chip. The I/O addresses are decoded by the Super-I/O chip itself,
> > and in general it has no relation to PCI. And I've never seen ports
> > 0x2e/0x2f nor 0x4e/0x4f listed in /proc/ioports.
> >
> > But of course if there is a way to know, we should use it. Avoiding
> > random access to I/O ports, even if they are relatively standard in
> > this case, is always good.
>
> I looked at my lspci output and did a little researching, and I think
> the only thing we can deduce is that there is an LPC bridge, so
> looking for a SuperIO is a good idea. If there is no LPC bridge
> listed, I can't say whether probing the ports is a good idea or not.
Machines I have here have an "ISA bridge" PCI device. Some of them have
"LPC" in their name, but not all, and anyway the kernel only knows the
device ID, not its user-friendly name:
VIA:
00:11.0 ISA bridge: VIA Technologies, Inc. VT8237 ISA bridge [KT600/K8T800/K8T890 South]
Intel:
00:01.0 ISA bridge: Intel Corporation 82371AB/EB/MB PIIX4 ISA (rev 01)
00:1f.0 ISA bridge: Intel Corporation 82801CAM ISA Bridge (LPC) (rev 01)
00:1f.0 ISA bridge: Intel Corporation 82801EB/ER (ICH5/ICH5R) LPC Interface Bridge (rev 02)
nVidia:
00:01.0 ISA bridge: nVidia Corporation nForce2 ISA Bridge (rev a4)
One of these machines has a LPC bridge but no (detected) Super-I/O chip.
The VIA and nVidia machines do not have "LPC" in their bridge name, but
they do have a Super-I/O chip, while the first Intel machine doesn't.
As a conclusion, there is no clear relationship between the presence of
an ISA or LPC bridge and the presence of a Super-I/O chip. All we can
say is that apparently all PC systems have an ISA bridge. So indeed we
can probably make the probes conditional upon the presence of an "ISA
bridge" PCI device. That's very easy to test. In practice it might be
equivalent to making the driver x86-only.
--
Jean Delvare
On 16-07-08 09:46, Jean Delvare wrote:
> As a conclusion, there is no clear relationship between the presence
> of an ISA or LPC bridge and the presence of a Super-I/O chip. All we
> can say is that apparently all PC systems have an ISA bridge. So
> indeed we can probably make the probes conditional upon the presence
> of an "ISA bridge" PCI device. That's very easy to test. In practice
> it might be equivalent to making the driver x86-only.
And foregoing non (pre-) PCI. Admittedly, that might not be much of an
issue and I don't know if "Super-I/O" is maybe even defined such as to
make it a NON issue but I do for example remember my old 386 chipset
providing for extended serial rates which I thought was way cool back
then. If that's Super-I/O (or something that should/could be covered by
the infrastructure at least) then PCI wouldn't be helpful.
Rene.