Re: [PATCH 3/3] ipmi/acpi: Install the IPMI space handler to enable ACPI to access the BMC controller

[ykzhao <yakui.zhao@intel.com>]

On Fri, 2009-12-18 at 06:00 +0800, Bjorn Helgaas wrote:
> On Wednesday 16 December 2009 06:52:00 pm ykzhao wrote:
> > On Thu, 2009-12-17 at 00:40 +0800, Bjorn Helgaas wrote:
> 
> > > > +     struct acpi_ipmi_device *ipmi_device =
> > > > +                     (struct acpi_ipmi_device *) handler_context;
> > > 
> > > I don't think you need this cast.
> > 
> > Without the initialization or cast type, it still can work. But it is
> > not wrong.
> > Right? Of course I can remove it.
> 
> The initialization and cast aren't *wrong*,  they're just unnecessary.
> In this case, it also makes it look different from other Linux code,
> which makes it harder to read.
> 
> > > > +static int acpi_ipmi_opregion_init(void)
> > > > +{
> > > > +     int result = 0;
> > > > +
> > > > +     if (acpi_disabled)
> > > > +             return result;
> > > 
> > > I don't think you need this check.
> > 
> > It is also ok to delete this check. 
> > But in fact it is unnecessary to register the smi_watcher again when
> > ACPI is disabled.  Otherwise after we register it, it will call the
> > corresponding callback defined in smi_watcher when one IPMI system
> > interface is registered/unregistered. 
> > 
> > So IMO we can register the smi_watcher only when ACPI is enabled.
> 
> This is connected with my opinion that you should call
> acpi_ipmi_opregion_init() from ipmi_pnp_probe(), not from the
> driver init function.
> 
> We only need the watcher when we have an IPI0001 device, so I
> don't see the point of registering it before we find one.  And if
> we find one, we know ACPI is enabled, so we don't need to check
> acpi_disabled.
> 
> > > > @@ -2096,17 +2576,33 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> > > >       info->dev = &acpi_dev->dev;
> > > >       pnp_set_drvdata(dev, info);
> > > >
> > > > -     return try_smi_init(info);
> > > > +     acpi_dev->driver_data = p_ipmi;
> > > 
> > > I don't think we should use both the pnp_dev driver_data
> > > and the acpi_dev driver_data.  There's only one underlying
> > > physical device, and the fact that we have two ways to get
> > > at it is a Linux design problem.  It would be better to add
> > > the p_ipmi pointer to the smi_info structure, and use only
> > > pnp_set_drvdata().
> > 
> > Understand what you said.
> > 
> > It is not a good idea to add a new pointer in smi_info structure. In
> > fact the smi_info is the abstract layer of IPMI system interface. To add
> > a definition in smi_info seems a bit confusing.
> 
> If you can figure out another place to put it, that's OK with me.
> I just don't want to use acpi_dev->driver_data because someday I
> want to get rid of the acpi_dev for PNP devices.  We shouldn't have
> both a pnp_dev and and acpi_dev for the same device.
> 
> > > > @@ -3207,6 +3703,7 @@ static __devinit int init_ipmi_si(void)
> > > >
> > > >  #ifdef CONFIG_ACPI
> > > >       spmi_find_bmc();
> > > > +     acpi_ipmi_opregion_init();
> > > 
> > > I don't think this belongs in the IPMI driver init.  It should be done
> > > in ipmi_pnp_probe() when we actually discover an IPI0001 device.
> > 
> > This function is mainly to register the smi_watcher. When one IPMI
> > system interface(smi_info) is registered/unregistered, it will call the
> > corresponding callback function to create/destroy the corresponding IPMI
> > user interface between the user and IPMI system interface.(In our code
> > the user is the ACPI IPMI device defined in ACPI namespace). 
> > That means that it can create the corresponding user interface for more
> > than one ACPI IPMI devices. 
> > So IMO this had better not be added in the ipmi_pnp_probe/remove
> > function.
> 
> We don't need the watcher unless we find an IPI0001 device, so I think
> it makes sense to put it in ipmi_pnp_probe().  You might have to have
> some static state so you only call acpi_ipmi_opregion_init() on the
> *first* probe and do the cleanup on the *last* call to ipmi_pnp_remove().

If no IPI0001 device is not found, it is unnecessary to register the
smi_watcher. When the IPI0001 device is found, we should register the
smi_watcher. I think that your suggestion is also OK. 

But if we put it outside of ipmi_pnp_probe/ipmp_pnp_remove, then we
don't care when and how to register/unregister it. In such case if a new
IPMI smi_info is registered, it will call this smi_watcher.

In fact when we call the function of ipmi_smi_watcher_register to
register the smi_watcher, it will do two things. One is to enumerate the
current smi_info list to call the callback function. The second is to
add this smi_watcher and when one IPMI smi_info is registered, it can be
called.

Of course I can remove the unmeaningful check of acpi_disabled.


> 
> Thanks for being so patient with all this.  I know I'm being picky,
> but it's much easier to fix up as much as we can before it goes into
> the tree.
> 
> Bjorn