bmcsensors

Greg has asked me to send it in-line to the list, so here it is :). I'd appreciate any comments on the patch, please read the previous threads on the sysfs callback problem first though.

Thanks,
Yani

---------- Forwarded message ----------
From: Yani Ioannou <[email protected]>
Date: Tue, 29 Mar 2005 15:39:39 -0500
Subject: Re: Fwd: [PATCH 2.6] bmcsensors
To: Greg KH <[email protected]>

Sorry I was following this section of Documentation/SubmittingPatches, the patch uncompressed is ~75K:

-----------------------Documentation/SubmittingPatches----------------------------- 210 Large changes are not appropriate for mailing lists, and some
211 maintainers. If your patch, uncompressed, exceeds 40kb in size, 212 it is preferred that you store your patch on an Internet-accessible 213 server, and provide instead a URL (link) pointing to your patch.

I've attached it in line below, sorry for any inconvenience.

Thanks,
Yani

-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch-linux-2.6.11.5-bmcsensors.diff
Type: text/x-patch
Size: 76104 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050330/0572e179/patch-linux-2.6.11.5-bmcsensors.bin

Hi Yani,

These rules are pretty much list- and area-dependent. I wouldn't send a 40k patch over LKML, but I wouldn't mind on the sensors list. Likewise, submitting patches against the latest -mm sounds like a good default rule, and is definitely the best choice for i2c, but for some other areas different trees may be prefered. I'm not sure where we could write down these rules for the hardware monitoring area. Thinks work rather well already anyway.

Jean Delvare

Hi Jean,

Oops, there was a signed off line in the first e-mail to the mailing list and I'd just assumed it was still in the forward. I will port the patch to the latest mm-series tonight and repost.

We should write all this stuff down somewhere :-) (cc Greg, post in-line even over 40k, patch against latest mm etc) for newbies like me, unless I'm missing some existing documentation somewhere (I read the kernel documentation, Documentation/SubmittingPatches, and the lm_sensors porting_clients documentation, etc).

Thanks,
Yani

Thanks Mark :),

In fact i2c-ipmi is virtually unchanged from 2.4 compared to the changes in bmcsensors (it should probably still have Mark as module author but I didn't want him having to answer questions from people on my code).

I know it seems like a hack - it is, but I think with reason. I agree with mds that it is nice to have a centralized system in Linux for accessing sensor information (regardless of it being on i2c or ipmi) through the lm_sensors sysfs interface, rather than use something like ipmitool to read BMC sensors and lm_sensors to read others, especially considering the amount of software out there already using the lm_sensors interface.

Certainly many people I've been in contact with seem to think this is true too (hence the demand for bmcsensors), and I'm all for giving people the choice. i2c-ipmi is in my opinion just an (unfortunate) way of fitting into the current lm_sensors driver model, there are other places in the patch that bmc_sensors differs from all the other i2c based drivers too and had to do its own thing for the same reason.

Yani

Don't pick on Yani too hard, he's just porting what I wrote. i2c-ipmi is indeed a bridge between bmcsensors and the IPMI subsystem. There's some documentation in doc/busses/i2c-ipmi and you can also look at the code in 2.4.

Yes it's a little bit of a hack but I thought this architecture was the best way to get IPMI data out through the sensors API. Sure there are other ways to do it and you could use some full-blown IPMI app but this is a good way to get people the info that gets hidden behind all the IPMI stuff - i.e. their sensor info.

I think the dummy driver approach has value for those who want to present data via the sensors API. As you may recall I did something similar for hard drive temperatures (S.M.A.R.T) a while back.

If you have a better way to do dummy bus drivers let's hear it but let's not ban dummy bus drivers (or new dummy bus drivers anyway) altogether.

mds

Hi Yani,

Please provide your patch against the latest -mm tree. This is where the latest i2c developments are. Your current patch doesn't apply against 2.6.12-rc1-mm3 (fails on drivers/i2c/chips/Makefile).

Note that i2c_client.id is no more, so this won't work:

+struct i2c_client bmc_client = {
+ .name = "bmc",
+ .id = 1, /* fake should be 0 */
+ .flags = 0,
+ .addr = 0,
+ .adapter = NULL, /* adapter */
+ .driver = &bmcsensors_driver,
+ .usage_count = 0
+};

And this won't work either, of course:

+ bmc_client.id = 0;

Why is bmc_client no declared static?

Could you please explain to us what i2c-ipmi does? I understand it acts as an interface between the bmcsensors driver and something else, but I yet have to learn what that other thing is, and why an interface is needed at all. As I understand it, i2c-ipmi is no I2C or SMBus master driver, but a kind of hack much like the i2c-isa driver is. I don't exactly enjoy the way i2c-isa works, and I plan to get rid of it at some point in time (hopefully in a near future), so let's not do something equally ugly here.

Thanks,
--
Jean Delvare

Hi all,

Actually trying to port i2c_ipmi over to 2.6.12-rc1-mm3 is not trivial
- the removal of i2c_client.id has broken it a lot and it looks like I'll have to add code to maintain a separate list of bmcsensors clients with ids (it was using the i2c_adapter.clients list along with the client ids to identify clients) if we want to keep i2c-ipmi in its present form.

Was client.id removed just because none of the ported drivers in the kernel was using it? A cursory glance at the 2.4 code (which includes i2c-ipmi) might have shown this isn't necessarily true for drivers that are in the process of being ported. Do any of the other sensors/busses yet to be ported to 2.6, or in the process, have a reliance on it?

MDS: i2c-ipmi seems to maintain the client list to allow the possibility of more than one bmcsensors driver to use the interface, however since bmcsensors doesn't support more than one bmc anyway should this functionality be removed instead of patched?

Thanks,
Yani

Hi Jean,

Thanks for your reply. I agree that changes need to be made - i2c-ipmi and i2c-isa *should not exist*. However the problem isn't that the original authors of i2c-isa and i2c-ipmi wrote bad code or had the wrong idea, the problem is that the lm_sensors driver model for hardware sensors was created on the assumption that all hardware drivers would be i2c driven, and has not changed despite the introduction of all sorts of non-i2c based hardware sensors over the years including isa and ipmi based, of which i2c-ipmi and i2c-isa try to address.

If you really want to do the right thing (c) what needs to change first and foremost is the lm_sensors hardware sensor model, the sensor access method has to be abstracted away from i2c into something more general, and other i2c-only legacies must also be abstracted if lm_sensors is going to be the centralized sensor framework in Linux (which I believe it should be).

I am perfectly willing to help in this effort and to re-write parts or all of the driver in the aim of doing the right thing, however we cannot just blindside all those people out there who want to access their ipmi based sensors now (likewise we can't just get rid of the isa based sensor functionality that many are using). The time to ditch i2c-ipmi/i2c-isa is the time we have a functional (and stable) alternative.

Similarly I am working on dynamic callbacks (which would seem at first glance to be an extremely simple patch to device.h), but as MDS suggested instead of waiting for this patch to be accepted (or not) and modify bmcsensors to work with it, why not submit what I have that people are using, is stable and works now.

The bmcsensors/i2c-ipmi 2.6 port is functional, stable and has been out and used by people for more than a year (and its 2.4 equivalent much longer), I have submitted it now due to the demand from the people who use it to have it included along with the rest of lm_sensors.

Quite simply people who have switched to 2.6 from 2.4 don't want to loose the ability to access their sensors waiting for us to re-implement the ipmi and isa functionality in a better way, and I don't think we should just abandon them in such a way.

Yani

Hi Yani,

I fully agree.

In a way, the abstraction layer already exists, it's the sysfs interface drivers export. Nothing prevents a non-i2c drivers from creating the same sysfs files with the same conventions. The only drawback is that userspace tools (libsensors in particular) look for chips only in /sys/bus/i2c at the moment, so we will need to extend these tools to look in different places. We will also need to find a way for libsensors to find out which drivers are hardware monitoring drivers. I think that there's something named classes in the sysfs model, maybe that's what we need? A new "hwmon" class of drivers?

Then we'll have to move a very little part of common hardware-monitoring-specific code away from i2c (i2c-sensor-vid.c and i2c-sensor.h). This ain't hard, we just need to find a new home. drivers/hwmon maybe? And rename the files, as they are not specific to i2c at all.

Then we'll have to attach non-i2c hardware monitoring drivers to where they belong. I am not sure I know where this is for the various isa drivers, and even less for bmcsensors. What "bus" would you attach it to? If there something like an IPMI bus?

I am not going to break the existing i2c-isa drivers (but I AM going to transform it radically, because that's needed). As for i2c-ipmi/bmcsensors, it's not yet in the kernel, and you provide patches for people who want them, so there's no need to worry. By delaying the integration of your patch into the kernel until some things have been cleaned up, we are not preventing anyone from using your patches.

Since I am currently trying to separate the i2c-isa specific code from the i2c core, I know what must not be done for i2c-ipmi/bmcsensors. i2c-ipmi must NOT register itself with i2c-core, neither must bmcsensors. Only i2c clients can be registered with the i2c core, because in a way clients are the abtraction layer you were talking about. I will have a very hard time extracting all the i2c-isa stuff from the i2c core just because i2c-isa was considered an almost normal i2c adapter, and was registered with the core. Let's not do the same error again.

Could you please post your proposed change to device.h? I'd sure like to take a look.

You are correct, let's not add dependencies where they do not exist.

(I would however create less nodes by default than you do. 100 is really much.)

Note however that the fact that your code "is stable and works" doesn't mean it'll get accepted. To be accepted, the code must work AND be technically correct. That's where the problem is, obviously.

As a side note, the 2.4 version is not exactly well maintained. i2c-ipmi has lots of warnings, and lm_sensors 2.9.0 was released with a bmcsensors driver which wouldn't even compile. Thus I seriously question the large userbase you are invoking.

Now I don't deny the fact that people asked you for the driver to be pushed in mainstream. But again this doesn't fly if the code is not something we can accept from a technical point of view.

Again they have your patches if they want, and this works just fine as you said. So let's not rush.

I have started working on the i2c-isa issue, and think I have a first working step in converting i2c-isa from a bus driver (dumb i2c_adapter) to an i2c-core-like thing for isa hardware monitoring drivers. This duplicates some code of course, and requires minor changes to 9 (isa) chip drivers, but this opens the door to a whole lot cleanups all around the place, both in the i2c core and in the isa-only drivers (6 of them). I'll work as fast as possible (partly depends on how much attention Greg can pay to this) and I hope to have a big series of patches ready by the end of the week. Once this is done, I think that things will be much more clear (well, less obscure at least), and the next steps to really make the isa drivers independant from the i2c core will be easier to find.

If you want to take a look, my preliminary patch is here (I improved it since, will update this evening):
http://jdelvare.net1.nerim.net/sensors/linux-2.6.12-rc1-bk5-i2c-isa-separated.diff

Some stats and a partial plan are here (needs update too):
http://jdelvare.net1.nerim.net/sensors/notes.txt

Once i2c-isa is done (even partly) we'll have a better idea of what can (and should) be done for i2c-ipmi/bmcsensors.

Thanks,
--
Jean Delvare

Hi Yani,

The i2c_client.id was removed because most of the existing drivers in 2.6 were not using it. The few of those which did were using it uniquely identify the client in log messages, but the various dev_* loggers already identify the device uniquely so it was redundant.

The reason why removing i2c_client.id was considered is because the i2c core doesn't use it. This means that, if a driver ever needs to keep a unique identifier on every client, it has to store it withing the client private data (i2c_client.data.id or whatever. This doesn't change much from this driver's point of view, but at least avoids including a mostly useless id field in all i2c clients in the kernel. The i2c_client.id removal killed dozens of (useless) lines of code and saved 4 bytes of memory per client. I'd do it again.

The problem you face here is possibly caused by the fact that i2c-ipmi and bmcsensors are not an i2c adapter driver and an i2c chip driver. They were hacked into the i2c subsystem because it sounded easy back then. I am continuously cleaning up the i2c subsystem in 2.6, step by step, and drivers which do not belong to this subsystem and are using hacks might break. I am not going to stop the cleanup process for them. Instead, they will have to be rewritten not to use random hacks.

The first candidate is i2c-isa. It is in no way an i2c adapter. The i2c core has various hacks to handle it because it is such a special case. As a result, even people using ISA-only hardware monitoring chips (most recent Super-I/O chip with integrated sensors fall into this category) still need to load the whole i2c subsystem. This doesn't make any sense, and I want to clean this up. This will make the i2c core much cleaner, will speed up the registration of i2c clients, and will open the path to even more cleanups and optimizations.

Now I understand that it doesn't exactly help you in your process of porting bmcsensors to Linux 2.6, and I am sorry about that. I also understand that you are not the author of the original i2c-ipmi+bmcsensors design, so I am not blaming you in any way for it. All I mean is that I want less hacks in the i2c subsystem, not more. I will work on the i2c-isa case and, if I can solve it, it should also provide hints on how the bmcsensors case can be solved. I will most probably need your assistance for this though, as I have next to zero knowledge on IPMI.

--
Jean Delvare