On Fri, 2010-10-15 at 00:53 +0800, Corey Minyard wrote:
> These are better, I'm ok with the other patches (well, I didn't review
> the ACPI changes, but I know little about that).
>
> You do have the refcounting stuff wrong, though. You only need a
> refcount if you receive a pointer to the information. For instance, you
> would have:
>
> int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
> struct ipmi_smi_info **data);
>
> and
>
> void ipmi_put_smi_info(struct ipmi_smi_info *data);
Thanks for the comments.
The updated structure of ipmi_smi_info will contain the dev pointer. In
order to assure that the dev pointer is safely accessed, the refcount of
dev will be increased/decreased. At the same time to avoid that the
caller increases/decrease the refcount explicitly, the wrapper is
added.
If the refcount of dev pointer is not considered in the caller, IMO the
refcount can be ignored.
>
> The way you are doing it, there is no need for a refcount, since you are
> making a copy of the data.
>
> Is a copy or a pointer better? A pointer is generally preferred, it
> keeps from having to either store data on the stack or dynamically
> allocate it for the copy. But it's not a huge deal in this case. A
> pointer will require you to dynamically allocate the smi_info structure
> so you can free it separately. But then only the top-level put routine
> is required, it can simply free the structure if the refcount is zero.
When the pointer mechanism is used, we will have to allocate the
smi_info structure dynamically. Every time the function of
ipmi_get_smi_info, it will be allocated dynamically. And if it fails in
the allocation, we can't return the expected value.
But when the copy is used, it will be much simpler. It is the caller's
responsibility to prepare the corresponding data structure. They can
define it on stack. Of course they can also dynamically allocate it.
Can we choose the copy mechanism to make it much simpler?
Thanks.
>
> -corey
>
> On 10/12/2010 02:47 AM, yakui.zhao@xxxxxxxxx wrote:
> > From: Zhao Yakui<yakui.zhao@xxxxxxxxx>
> >
> > The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
> > In order to communicate with the correct IPMI device, it should be confirmed
> > whether it is what we wanted especially on the system with multiple IPMI
> > devices. But the new_smi callback function of smi_watcher provides very
> > limited info(only the interface number and dev pointer) and there is no
> > detailed info about the low level interface. For example: which mechansim
> > registers the IPMI interface(ACPI, PCI, DMI and so on).
> >
> > This is to add one interface that can get more info of low-level IPMI
> > device. For example: the ACPI device handle will be returned for the pnp_acpi
> > IPMI device.
> >
> > Signed-off-by: Zhao Yakui<yakui.zhao@xxxxxxxxx>
> > ---
> > In this patch only the info of pnp_acpi IPMI is provided. If the detailed info
> > is also required for other mechanism, please add them in the structure of
> > ipmi_smi_info.
> >
> > drivers/char/ipmi/ipmi_msghandler.c | 47 +++++++++++++++++++++++++++++++++++
> > drivers/char/ipmi/ipmi_si_intf.c | 35 +++++++++++++++++++++++---
> > include/linux/ipmi.h | 25 ++++++++++++++++++
> > include/linux/ipmi_smi.h | 12 +++++++++
> > 4 files changed, 115 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> > index 4f3f8c9..29e373d 100644
> > --- a/drivers/char/ipmi/ipmi_msghandler.c
> > +++ b/drivers/char/ipmi/ipmi_msghandler.c
> > @@ -970,6 +970,53 @@ out_kfree:
> > }
> > EXPORT_SYMBOL(ipmi_create_user);
> >
> > +int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
> > + struct ipmi_smi_info *data)
> > +{
> > + int rv = 0;
> > + ipmi_smi_t intf;
> > + struct ipmi_smi_handlers *handlers;
> > +
> > + mutex_lock(&ipmi_interfaces_mutex);
> > + list_for_each_entry_rcu(intf,&ipmi_interfaces, link) {
> > + if (intf->intf_num == if_num)
> > + goto found;
> > + }
> > + /* Not found, return an error */
> > + rv = -EINVAL;
> > + mutex_unlock(&ipmi_interfaces_mutex);
> > + return rv;
> > +
> > +found:
> > + handlers = intf->handlers;
> > + rv = handlers->get_smi_info(intf->send_info, type, data);
> > + mutex_unlock(&ipmi_interfaces_mutex);
> > +
> > + return rv;
> > +}
> > +EXPORT_SYMBOL(ipmi_get_smi_info);
> > +
> > +void ipmi_put_smi_info(int if_num)
> > +{
> > + ipmi_smi_t intf;
> > + struct ipmi_smi_handlers *handlers;
> > +
> > + mutex_lock(&ipmi_interfaces_mutex);
> > + list_for_each_entry_rcu(intf,&ipmi_interfaces, link) {
> > + if (intf->intf_num == if_num)
> > + goto found;
> > + }
> > + /* Not found, return */
> > + mutex_unlock(&ipmi_interfaces_mutex);
> > + return;
> > +
> > +found:
> > + handlers = intf->handlers;
> > + handlers->put_smi_info(intf->send_info);
> > + mutex_unlock(&ipmi_interfaces_mutex);
> > +}
> > +EXPORT_SYMBOL(ipmi_put_smi_info);
> > +
> > static void free_user(struct kref *ref)
> > {
> > ipmi_user_t user = container_of(ref, struct ipmi_user, refcount);
> > diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> > index 7bd7c45..4885709 100644
> > --- a/drivers/char/ipmi/ipmi_si_intf.c
> > +++ b/drivers/char/ipmi/ipmi_si_intf.c
> > @@ -57,6 +57,7 @@
> > #include<asm/irq.h>
> > #include<linux/interrupt.h>
> > #include<linux/rcupdate.h>
> > +#include<linux/ipmi.h>
> > #include<linux/ipmi_smi.h>
> > #include<asm/io.h>
> > #include "ipmi_si_sm.h"
> > @@ -107,10 +108,6 @@ enum si_type {
> > };
> > static char *si_to_str[] = { "kcs", "smic", "bt" };
> >
> > -enum ipmi_addr_src {
> > - SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> > - SI_PCI, SI_DEVICETREE, SI_DEFAULT
> > -};
> > static char *ipmi_addr_src_to_str[] = { NULL, "hotmod", "hardcoded", "SPMI",
> > "ACPI", "SMBIOS", "PCI",
> > "device-tree", "default" };
> > @@ -291,6 +288,7 @@ struct smi_info {
> > struct task_struct *thread;
> >
> > struct list_head link;
> > + struct ipmi_smi_info smi_data;
> > };
> >
> > #define smi_inc_stat(smi, stat) \
> > @@ -1186,6 +1184,31 @@ static int smi_start_processing(void *send_info,
> > return 0;
> > }
> >
> > +static int get_smi_info(void *send_info,
> > + enum ipmi_addr_src type,
> > + struct ipmi_smi_info *data)
> > +{
> > + struct smi_info *new_smi = send_info;
> > + struct ipmi_smi_info *smi_data =&new_smi->smi_data;
> > +
> > + if (new_smi->addr_source != type)
> > + return -EINVAL;
> > +
> > + get_device(new_smi->dev);
> > + memcpy(data, smi_data, sizeof(*smi_data));
> > + smi_data->addr_src = type;
> > + smi_data->dev = new_smi->dev;
> > +
> > + return 0;
> > +}
> > +
> > +static void put_smi_info(void *send_info)
> > +{
> > + struct smi_info *new_smi = send_info;
> > +
> > + put_device(new_smi->dev);
> > +}
> > +
> > static void set_maintenance_mode(void *send_info, int enable)
> > {
> > struct smi_info *smi_info = send_info;
> > @@ -1197,6 +1220,8 @@ static void set_maintenance_mode(void *send_info, int enable)
> > static struct ipmi_smi_handlers handlers = {
> > .owner = THIS_MODULE,
> > .start_processing = smi_start_processing,
> > + .get_smi_info = get_smi_info,
> > + .put_smi_info = put_smi_info,
> > .sender = sender,
> > .request_events = request_events,
> > .set_maintenance_mode = set_maintenance_mode,
> > @@ -2143,6 +2168,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> > return -ENOMEM;
> >
> > info->addr_source = SI_ACPI;
> > + info->smi_data.smi_info.acpi_info.acpi_handle =
> > + acpi_dev->handle;
> > printk(KERN_INFO PFX "probing via ACPI\n");
> >
> > handle = acpi_dev->handle;
> > diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
> > index 65aae34..2d30551 100644
> > --- a/include/linux/ipmi.h
> > +++ b/include/linux/ipmi.h
> > @@ -454,6 +454,31 @@ unsigned int ipmi_addr_length(int addr_type);
> > /* Validate that the given IPMI address is valid. */
> > int ipmi_validate_addr(struct ipmi_addr *addr, int len);
> >
> > +enum ipmi_addr_src {
> > + SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> > + SI_PCI, SI_DEVICETREE, SI_DEFAULT
> > +};
> > +struct ipmi_smi_info{
> > + enum ipmi_addr_src addr_src;
> > + struct device *dev;
> > + union {
> > + /*
> > + * Now only SI_ACPI info is provided. If the info is required
> > + * for other type, please add it
> > + */
> > +#ifdef CONFIG_ACPI
> > + struct {
> > + void *acpi_handle;
> > + } acpi_info;
> > +#endif
> > + } smi_info;
> > +};
> > +
> > +/* This is to get the private info of ipmi_smi_t */
> > +extern int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
> > + struct ipmi_smi_info *data);
> > +/* This is to decrease refcount added in the function of ipmi_get_smi_info */
> > +extern void ipmi_put_smi_info(int if_num);
> > #endif /* __KERNEL__ */
> >
> >
> > diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
> > index 4b48318..b9a53c0 100644
> > --- a/include/linux/ipmi_smi.h
> > +++ b/include/linux/ipmi_smi.h
> > @@ -86,6 +86,18 @@ struct ipmi_smi_handlers {
> > int (*start_processing)(void *send_info,
> > ipmi_smi_t new_intf);
> >
> > + /*
> > + * Get the detailed private info of the low level interface and store
> > + * it into the structure of ipmi_smi_data. For example: the
> > + * ACPI device handle will be returned for the pnp_acpi IPMI device.
> > + * Of course it will firstly compare the interface type of low-level
> > + * interface(Hardcoded type, DMI, ACPI and so on). If the type doesn't
> > + * match, it will return -EINVAL.
> > + */
> > + int (*get_smi_info)(void *send_info, enum ipmi_addr_src type,
> > + struct ipmi_smi_info *data);
> > +
> > + void (*put_smi_info)(void *send_info);
> > /* Called to enqueue an SMI message to be sent. This
> > operation is not allowed to fail. If an error occurs, it
> > should report back the error in a received message. It may
> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
