Re: [PATCH v3 2/9] hwmon: (core) New hwmon registration API

[Jean Delvare]

Hi Guenter,

On Mon, 10 Oct 2016 16:48:22 -0700, Guenter Roeck wrote:
> On Fri, Oct 07, 2016 at 02:32:13PM +0200, Jean Delvare wrote:
> > On Tue, 4 Oct 2016 12:37:13 -0700, Guenter Roeck wrote:
> > > On Tue, Oct 04, 2016 at 10:45:59AM +0200, Jean Delvare wrote:
> > > > May I ask if any other subsystem is already doing anything like that?
> > > > 
> > > Depends what you mean with "anything like that". The API is inspired by
> > > the iio API and a little by the thermal API. The details are probably
> > > unique, though.
> > 
> > I meant the idea of describing the device capabilities and letting the
> > core code generate the device attributes (or anything else) based on
> > that data. The benefits are obvious, but it has a complexity and
> > run-time cost so I am wondering how popular this approach is.
> 
> The goal of this patch series was to provide abstraction between driver
> and userspace ABI. I think this is quite common in the kernel.

Yes, you are right. I am so used to the (wrong) way hwmon did that I
did not realize all other subsystems already provided some form of
abstraction for their device drivers. Even i2c does, so I should know.

> I am not sure I undersatand the run-time cost concern. The main run-time
> cost occurs during device instantiation and only happens once. Other
> abstraction APIs in the kernel have a much higher runtime cost, and
> instantiation tends to be quite costly as well.

I'm not too worried about this one, except for the memory allocations
as mentioned previously.

I am more concerned about the per-sysfs read overhead. Compared to the
old model, you have another indirection call for each access. And for
each driver, you end up with a single function to read from all
attributes. You will inevitably end up with a huge switch/case
statement to figure out what value to return. This smells like linear
search? Not sure if the compiler can optimize it. I was also wondering
how cache-friendly such a large function can be.

But anyway, I didn't measure anything, it's pure speculation on my
side. And it's probably irrelevant as your change has many benefits
anyway. And I wouldn't even ask myself the question if things had been
implemented right in the first place. So just ignore me ^^

> The new API has the addd benefit of reducing driver size, in some cases
> significantly. Maybe I am off, but I considered this important, which is
> why I mentioned it. Maybe I should not have mentioned it at all, and
> instead have focused on abstraction alone.

You were very right mentioning it, I mention it every time a patch of
mine reduces binary size. That's one of the actual benefits of the
change, no reason to silent it.

> > You mean at the binary level?
> > 
> > Have you considered the case where several devices exist for the same
> > driver? Static attributes are shared between devices, but with the
> > generated ones this is no longer the case, right?
>
> As mentioned above, I considered static sizes to be more important.
> Sure, there are situations where multiple instances of a driver are
> loaded, but those are not typically memory size limited. But, again,
> I guess maybe I should not have mentioned driver size impact at all.

I did not make any measurement, and I won't take the time to make them.
So I have no idea how the various costs compare to each other. Which in
turn means I don't have any point here, and we can go with what you
have ;-)

Really I was only commenting out loud while reviewing. Doesn't mean
much...

> > > > (...)
> > > > This is adding a 4th and 5th way to register a hwmon device. Starts
> > > > being a bit messy... Do you have any plans to get rid of some of the
> > > > other functions to make things more consistent and efficient?
> > >
> > > Would be nice, but then someone would have to spend the time cleaning
> > > up the old drivers to replace the old API, and for the most part we would
> > > not be able to test the result. Are you sure that is worth the risk ?
> > 
> > What I had in mind was rather along the lines of marking the function
> > as __deprecated, adding a run-time notice message when it is called,
> > and let whoever uses these driver contribute the fix. Call me an
> > optimistic :-) There are 54 drivers still using
> > hwmon_device_register(), out of 157.
>
> For most of those testing was not possible, otherwise I would have converted
> them by now.
> 
> I am not sure about deprecated; doesn't that mean a failure to compile with
> -Werror ? That would not help much.

It causes gcc to generate a warning, so I guess it would indeed break
with -Werror. But this option isn't enabled by default, and I doubt
anyone can actually build a general-purpose kernel with that option
enabled. If it's a custom kernel then maybe there's one hwmon driver
enabled, and if that one needs conversion, then you've just found your
tester ;-)

If you are worried about build-time __deprecated, you can also just
spit a message in the logs when hwmon_device_register() 

Re: [PATCH v3 2/9] hwmon: (core) New hwmon registration API

[Guenter Roeck]

On Fri, Oct 07, 2016 at 02:32:13PM +0200, Jean Delvare wrote:
> Hi Guenter,
> 
> On Tue, 4 Oct 2016 12:37:13 -0700, Guenter Roeck wrote:
> > On Tue, Oct 04, 2016 at 10:45:59AM +0200, Jean Delvare wrote:
> > > I see this patch is upstream now, but I had started reviewing it and
> > > maybe some of my comments are still relevant.
> > > 
> > As always, I appreciate the feedback. I'll be happy to submit follow-up
> > patches to address any concerns.
> > 
> > > It's not meant to be a complete review, which is why I had not sent it
> > > yet :-(
> > > 
> > > Also I did not follow the first iterations of this patchset so my
> > > apologies if I raise points which have already been discussed.
> > > 
> > > May I ask if any other subsystem is already doing anything like that?
> > > 
> > Depends what you mean with "anything like that". The API is inspired by
> > the iio API and a little by the thermal API. The details are probably
> > unique, though.
> 
> I meant the idea of describing the device capabilities and letting the
> core code generate the device attributes (or anything else) based on
> that data. The benefits are obvious, but it has a complexity and
> run-time cost so I am wondering how popular this approach is.
> 

The goal of this patch series was to provide abstraction between driver
and userspace ABI. I think this is quite common in the kernel.

I am not sure I undersatand the run-time cost concern. The main run-time
cost occurs during device instantiation and only happens once. Other
abstraction APIs in the kernel have a much higher runtime cost, and
instantiation tends to be quite costly as well.

The new API has the addd benefit of reducing driver size, in some cases
significantly. Maybe I am off, but I considered this important, which is
why I mentioned it. Maybe I should not have mentioned it at all, and
instead have focused on abstraction alone.

> You mean at the binary level?
> 
> Have you considered the case where several devices exist for the same
> driver? Static attributes are shared between devices, but with the
> generated ones this is no longer the case, right?
> 
As mentioned above, I considered static sizes to be more important.
Sure, there are situations where multiple instances of a driver are
loaded, but those are not typically memory size limited. But, again,
I guess maybe I should not have mentioned driver size impact at all.

> > > > +
> > > > +   mode = ops->is_visible(drvdata, type, attr, index);
> > > 
> > > Any reason why you don't simply attach the provided ->is_visible to the
> > > attribute group and let the driver core do the work?
> > > 
> > Parameter are all different
> > umode_t (*is_visible)(const void *drvdata, enum hwmon_sensor_types type,
> >   u32 attr, int channel);
> > vs.
> > umode_t (*is_visible)(struct kobject *, struct attribute *, int);
> 
> But this is the consequence of how you implemented it, not the reason?
> 
Not really. The drivers are now abstracted from the ABI and don't know anything
about it. Effectively I would need a shim is_visible function to convert the
sysfs parameters into driver function parameters. That should still be possible,
I just don't immediately see the benefits.

> Now I seem to understand that delegating the check to the driver core
> would make it happen later, which means you would have to instantiate
> all attributes, even if some are never going to be used? So this is an
> optimization?
> 
Not for that purpose. I didn't even get to that point because I did not see
a value in the shim function mentioned above.

> But this prevents sharing the attributes between devices, as pointed
> out above. Well, I guess you can't have it all. I don't know what is
> the more important.
> 
> > > > +   return ERR_PTR(-ENOENT);
> > > > +
> > > > +   if ((mode & S_IRUGO) && !ops->read)
> > > > +   return ERR_PTR(-EINVAL);
> > > > +   if ((mode & S_IWUGO) && !ops->write)
> > > > +   return ERR_PTR(-EINVAL);
> > > > +
> > > > +   if (type == hwmon_chip) {
> > > 
> > > This needs a comment. It's not obvious what "hwmon_chip" is supposed to
> > > represent. From what I understand, maybe "hwmon_unique" would be a more
> > > appropriate name.
> > > 
> > Those are meant to be for attributes which apply to the entire chip.
> 
> Not really. As you implemented it, it is meant for attributes which are
> not specific to an input or output in particular. That is, attributes
> which do not include a channel number. "update_interval" and
> "alarms" (which I'd like to get rid of, btw) apply to the entire chip,
> but "temp_reset_history" does not.
> 
> > Not sure if 'hwmon_unique' would reflect that better.
> > From the documentation:
> > "A virtual sensor type, used to describe attributes
> >  which apply to the entire chip"
> > 
> > Do you have an idea for a better description ?
> 
> For one thing I would rename hwmon_chip_attr_templates to
> hwmon_chip_attrs 

Re: [PATCH v3 2/9] hwmon: (core) New hwmon registration API

[Jean Delvare]

Hi Guenter,

On Tue, 4 Oct 2016 12:37:13 -0700, Guenter Roeck wrote:
> On Tue, Oct 04, 2016 at 10:45:59AM +0200, Jean Delvare wrote:
> > I see this patch is upstream now, but I had started reviewing it and
> > maybe some of my comments are still relevant.
> > 
> As always, I appreciate the feedback. I'll be happy to submit follow-up
> patches to address any concerns.
> 
> > It's not meant to be a complete review, which is why I had not sent it
> > yet :-(
> > 
> > Also I did not follow the first iterations of this patchset so my
> > apologies if I raise points which have already been discussed.
> > 
> > May I ask if any other subsystem is already doing anything like that?
> > 
> Depends what you mean with "anything like that". The API is inspired by
> the iio API and a little by the thermal API. The details are probably
> unique, though.

I meant the idea of describing the device capabilities and letting the
core code generate the device attributes (or anything else) based on
that data. The benefits are obvious, but it has a complexity and
run-time cost so I am wondering how popular this approach is.

> > FYI I gave a presentation about the hwmon device registration API
> > evolution last week at Kernel Recipes [1] in Paris and mentioned this
> > proposal.
> > 
> > [1] https://kernel-recipes.org/en/2016/
> > 
> > On dim., 2016-07-24 at 20:32 -0700, Guenter Roeck wrote:
> > > Up to now, each hwmon driver has to implement its own sysfs attributes.
> > > This requires a lot of template code, and distracts from the driver's core
> > > function to read and write chip registers.
> > > 
> > > To be able to reduce driver complexity, move sensor attribute handling
> > > and thermal zone registration into hwmon core. By using the new API,
> > > driver code and data size is typically reduced by 20-70%, depending
> > > on driver complexity and the number of sysfs attributes supported.
> > 
> > I looked at the diffstats for the drivers you have already converted and
> > couldn't see any such huge improvement... Some drivers appear to be even
> > larger after conversion?
> > 
> The above refers to code and data sizes.

You mean at the binary level?

Have you considered the case where several devices exist for the same
driver? Static attributes are shared between devices, but with the
generated ones this is no longer the case, right?

> > > (...)
> > > +static struct attribute *hwmon_genattr(struct device *dev,
> > > +const void *drvdata,
> > > +enum hwmon_sensor_types type,
> > > +u32 attr,
> > > +int index,
> > > +const char *template,
> > > +const struct hwmon_ops *ops)
> > > +{
> > > + struct hwmon_device_attribute *hattr;
> > > + struct device_attribute *dattr;
> > > + struct attribute *a;
> > > + umode_t mode;
> > > + char *name;
> > > +
> > > + /* The attribute is invisible if there is no template string */
> > > + if (!template)
> > > + return ERR_PTR(-ENOENT);
> > > +
> > > + mode = ops->is_visible(drvdata, type, attr, index);
> > 
> > Any reason why you don't simply attach the provided ->is_visible to the
> > attribute group and let the driver core do the work?
> > 
> Parameter are all different
>   umode_t (*is_visible)(const void *drvdata, enum hwmon_sensor_types type,
> u32 attr, int channel);
> vs.
>   umode_t (*is_visible)(struct kobject *, struct attribute *, int);

But this is the consequence of how you implemented it, not the reason?

Now I seem to understand that delegating the check to the driver core
would make it happen later, which means you would have to instantiate
all attributes, even if some are never going to be used? So this is an
optimization?

But this prevents sharing the attributes between devices, as pointed
out above. Well, I guess you can't have it all. I don't know what is
the more important.

> > > + return ERR_PTR(-ENOENT);
> > > +
> > > + if ((mode & S_IRUGO) && !ops->read)
> > > + return ERR_PTR(-EINVAL);
> > > + if ((mode & S_IWUGO) && !ops->write)
> > > + return ERR_PTR(-EINVAL);
> > > +
> > > + if (type == hwmon_chip) {
> > 
> > This needs a comment. It's not obvious what "hwmon_chip" is supposed to
> > represent. From what I understand, maybe "hwmon_unique" would be a more
> > appropriate name.
> > 
> Those are meant to be for attributes which apply to the entire chip.

Not really. As you implemented it, it is meant for attributes which are
not specific to an input or output in particular. That is, attributes
which do not include a channel number. "update_interval" and
"alarms" (which I'd like to get rid of, btw) apply to the entire chip,
but "temp_reset_history" does not.

> Not sure if 'hwmon_unique' would reflect that better.
> From the documentation:
>   "A virtual sensor type, used to describe attributes

Re: [PATCH v3 2/9] hwmon: (core) New hwmon registration API

[Guenter Roeck]

Hi Jean,

On Tue, Oct 04, 2016 at 10:45:59AM +0200, Jean Delvare wrote:
> Hi Guenter,
> 
> I see this patch is upstream now, but I had started reviewing it and
> maybe some of my comments are still relevant.
> 
As always, I appreciate the feedback. I'll be happy to submit follow-up
patches to address any concerns.

> It's not meant to be a complete review, which is why I had not sent it
> yet :-(
> 
> Also I did not follow the first iterations of this patchset so my
> apologies if I raise points which have already been discussed.
> 
> May I ask if any other subsystem is already doing anything like that?
> 
Depends what you mean with "anything like that". The API is inspired by
the iio API and a little by the thermal API. The details are probably
unique, though.

> FYI I gave a presentation about the hwmon device registration API
> evolution last week at Kernel Recipes [1] in Paris and mentioned this
> proposal.
> 
> [1] https://kernel-recipes.org/en/2016/
> 
> On dim., 2016-07-24 at 20:32 -0700, Guenter Roeck wrote:
> > Up to now, each hwmon driver has to implement its own sysfs attributes.
> > This requires a lot of template code, and distracts from the driver's core
> > function to read and write chip registers.
> > 
> > To be able to reduce driver complexity, move sensor attribute handling
> > and thermal zone registration into hwmon core. By using the new API,
> > driver code and data size is typically reduced by 20-70%, depending
> > on driver complexity and the number of sysfs attributes supported.
> 
> I looked at the diffstats for the drivers you have already converted and
> couldn't see any such huge improvement... Some drivers appear to be even
> larger after conversion?
> 
The above refers to code and data sizes.

> > With this patch, the new API only supports thermal sensors. Support for
> > other sensor types will be added with subsequent patches.
> > 
> > Acked-by: Punit Agrawal 
> > Reviewed-by: Jonathan Cameron 
> > Signed-off-by: Guenter Roeck 
> > ---
> > v3:
> > - Thermal registration depends on IS_REACHABLE(CONFIG_THERMAL) and
> >   CONFIG_THERMAL_OF.
> > v2:
> > - Document callback function parameters of struct hwmon_ops in
> >   include/linux/hwmon.h.
> > - Clarify that the is_visible() callback is mandatory.
> > - Initialize device attribute read/write callback functions unconditionally.
> > - If an attribute has no template string, treat it as invisible, not as
> >   error. Affected are virtual attributes such as HWMON_C_REGISTER_TZ.
> > - Added newline to improve readability.
> > 
> > Review comments not addressed:
> > - Stick with u32 for attribute masks. We could use u64, but it is currently
> >   not needed, and changing it later would be straightforward.
> > - Do not use for_each_set_bit() to walk attribute masks.
> >   for_each_set_bit() expects a pointer to an unsigned long as argument,
> >   which would make it difficult to switch to u64 attribute masks if/when
> >   needed.
> > 
> >  drivers/hwmon/hwmon.c | 485 
> > +++---
> >  include/linux/hwmon.h | 148 +++
> >  2 files changed, 606 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> > index 649a68d119b4..3e4cc442a089 100644
> > --- a/drivers/hwmon/hwmon.c
> > +++ b/drivers/hwmon/hwmon.c
> 
> I believe it would make sense to add you copyright at the top of the
> file. That's a pretty big change with lots of code.
> 
I don't really feel too strong about that. But, sure, can do.

> > @@ -12,6 +12,7 @@
> >  
> >  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >  
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -21,6 +22,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #define HWMON_ID_PREFIX "hwmon"
> >  #define HWMON_ID_FORMAT HWMON_ID_PREFIX "%d"
> > @@ -28,9 +30,35 @@
> >  struct hwmon_device {
> > const char *name;
> > struct device dev;
> > +   const struct hwmon_chip_info *chip;
> > +
> > +   struct attribute_group group;
> > +   const struct attribute_group **groups;
> >  };
> > +
> >  #define to_hwmon_device(d) container_of(d, struct hwmon_device, dev)
> >  
> > +struct hwmon_device_attribute {
> > +   struct device_attribute dev_attr;
> > +   const struct hwmon_ops *ops;
> > +   enum hwmon_sensor_types type;
> > +   u32 attr;
> > +   int index;
> > +};
> > +
> > +#define to_hwmon_attr(d) \
> > +   container_of(d, struct hwmon_device_attribute, dev_attr)
> > +
> > +/*
> > + * Thermal zone information
> > + * In addition to the reference to the hwmon device,
> > + * also provides the sensor index.
> > + */
> > +struct hwmon_thermal_data {
> > +   struct hwmon_device *hwdev; /* Reference to hwmon device */
> > +   int index;  /* sensor index */
> > +};
> > +
> >  static ssize_t
> >  show_name(struct device *dev, struct device_attribute *attr, char *buf)
> >  {
> > @@ -78,25 

Re: [PATCH v3 2/9] hwmon: (core) New hwmon registration API

[Jean Delvare]

Hi Guenter,

I see this patch is upstream now, but I had started reviewing it and
maybe some of my comments are still relevant.

It's not meant to be a complete review, which is why I had not sent it
yet :-(

Also I did not follow the first iterations of this patchset so my
apologies if I raise points which have already been discussed.

May I ask if any other subsystem is already doing anything like that?

FYI I gave a presentation about the hwmon device registration API
evolution last week at Kernel Recipes [1] in Paris and mentioned this
proposal.

[1] https://kernel-recipes.org/en/2016/

On dim., 2016-07-24 at 20:32 -0700, Guenter Roeck wrote:
> Up to now, each hwmon driver has to implement its own sysfs attributes.
> This requires a lot of template code, and distracts from the driver's core
> function to read and write chip registers.
> 
> To be able to reduce driver complexity, move sensor attribute handling
> and thermal zone registration into hwmon core. By using the new API,
> driver code and data size is typically reduced by 20-70%, depending
> on driver complexity and the number of sysfs attributes supported.

I looked at the diffstats for the drivers you have already converted and
couldn't see any such huge improvement... Some drivers appear to be even
larger after conversion?

> With this patch, the new API only supports thermal sensors. Support for
> other sensor types will be added with subsequent patches.
> 
> Acked-by: Punit Agrawal 
> Reviewed-by: Jonathan Cameron 
> Signed-off-by: Guenter Roeck 
> ---
> v3:
> - Thermal registration depends on IS_REACHABLE(CONFIG_THERMAL) and
>   CONFIG_THERMAL_OF.
> v2:
> - Document callback function parameters of struct hwmon_ops in
>   include/linux/hwmon.h.
> - Clarify that the is_visible() callback is mandatory.
> - Initialize device attribute read/write callback functions unconditionally.
> - If an attribute has no template string, treat it as invisible, not as
>   error. Affected are virtual attributes such as HWMON_C_REGISTER_TZ.
> - Added newline to improve readability.
> 
> Review comments not addressed:
> - Stick with u32 for attribute masks. We could use u64, but it is currently
>   not needed, and changing it later would be straightforward.
> - Do not use for_each_set_bit() to walk attribute masks.
>   for_each_set_bit() expects a pointer to an unsigned long as argument,
>   which would make it difficult to switch to u64 attribute masks if/when
>   needed.
> 
>  drivers/hwmon/hwmon.c | 485 
> +++---
>  include/linux/hwmon.h | 148 +++
>  2 files changed, 606 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 649a68d119b4..3e4cc442a089 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c

I believe it would make sense to add you copyright at the top of the
file. That's a pretty big change with lots of code.

> @@ -12,6 +12,7 @@
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -21,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define HWMON_ID_PREFIX "hwmon"
>  #define HWMON_ID_FORMAT HWMON_ID_PREFIX "%d"
> @@ -28,9 +30,35 @@
>  struct hwmon_device {
>   const char *name;
>   struct device dev;
> + const struct hwmon_chip_info *chip;
> +
> + struct attribute_group group;
> + const struct attribute_group **groups;
>  };
> +
>  #define to_hwmon_device(d) container_of(d, struct hwmon_device, dev)
>  
> +struct hwmon_device_attribute {
> + struct device_attribute dev_attr;
> + const struct hwmon_ops *ops;
> + enum hwmon_sensor_types type;
> + u32 attr;
> + int index;
> +};
> +
> +#define to_hwmon_attr(d) \
> + container_of(d, struct hwmon_device_attribute, dev_attr)
> +
> +/*
> + * Thermal zone information
> + * In addition to the reference to the hwmon device,
> + * also provides the sensor index.
> + */
> +struct hwmon_thermal_data {
> + struct hwmon_device *hwdev; /* Reference to hwmon device */
> + int index;  /* sensor index */
> +};
> +
>  static ssize_t
>  show_name(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> @@ -78,25 +106,286 @@ static struct class hwmon_class = {
>  
>  static DEFINE_IDA(hwmon_ida);
>  
> -/**
> - * hwmon_device_register_with_groups - register w/ hwmon
> - * @dev: the parent device
> - * @name: hwmon name attribute
> - * @drvdata: driver data to attach to created device
> - * @groups: List of attribute groups to create
> - *
> - * hwmon_device_unregister() must be called when the device is no
> - * longer needed.
> - *
> - * Returns the pointer to the new device.
> - */
> -struct device *
> -hwmon_device_register_with_groups(struct device *dev, const char *name,
> -   void *drvdata,
> -   const struct 

Re: [PATCH v3 2/9] hwmon: (core) New hwmon registration API

[Guenter Roeck]

On 08/12/2016 01:16 AM, Keerthy wrote:

On Monday 25 July 2016 09:02 AM, Guenter Roeck wrote:

Up to now, each hwmon driver has to implement its own sysfs attributes.
This requires a lot of template code, and distracts from the driver's core
function to read and write chip registers.

To be able to reduce driver complexity, move sensor attribute handling
and thermal zone registration into hwmon core. By using the new API,
driver code and data size is typically reduced by 20-70%, depending
on driver complexity and the number of sysfs attributes supported.

With this patch, the new API only supports thermal sensors. Support for
other sensor types will be added with subsequent patches.


Hi Guenter,

Seems like this patch introduces a cycling dependency between
thermal_sys and hwmon:

DEPMOD 4.8.0-rc1-next-20160811-1-g2f84cf7854e7
depmod: ERROR: Found 2 modules in dependency cycles!
depmod: ERROR: Cycle detected: hwmon -> thermal_sys -> hwmon

As reported by R, Vignesh .



Hmm ... interesting. Thanks for letting me know. I'll have to think about that.

Guenter

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