Re: [PATCH v2 10/21] ipmi: kcs_bmc: Turn the driver data-structures inside-out
On Sat, 10 Apr 2021, at 04:56, Zev Weiss wrote: > On Fri, Apr 09, 2021 at 01:25:26AM CDT, Zev Weiss wrote: > >On Fri, Apr 09, 2021 at 12:59:09AM CDT, Andrew Jeffery wrote: > >>On Fri, 9 Apr 2021, at 13:27, Zev Weiss wrote: > >>>On Fri, Mar 19, 2021 at 01:27:41AM CDT, Andrew Jeffery wrote: > -struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv, > u32 channel); > -struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv, > u32 channel) > +int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc *kcs_bmc); > >>> > >>>Errant declaration again? > >> > >>As previously explained. > >> > > > >This one seems like a slightly different category, because... > > > >>> > +int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc *kcs_bmc) > > > >...it's immediately followed by the definition of the very same function > >that it just declared, so I can't see how its presence or absence could > >make any functional difference to anything. (So perhaps I should have > >said "redundant" instead of "errant...again".) This is is a small hack to fend off warnings from -Wmissing-declarations. > > > >It's fairly trivial of course given that it's gone by the end of the > >series, but as long as there's going to be another iteration anyway it > >seems like we might as well tidy it up? > > > > Oh, and otherwise: > > Reviewed-by: Zev Weiss Thanks. Andrew
Re: [PATCH v2 10/21] ipmi: kcs_bmc: Turn the driver data-structures inside-out
On Fri, Apr 09, 2021 at 01:25:26AM CDT, Zev Weiss wrote:
>On Fri, Apr 09, 2021 at 12:59:09AM CDT, Andrew Jeffery wrote:
>>
>>
>>On Fri, 9 Apr 2021, at 13:27, Zev Weiss wrote:
>>>On Fri, Mar 19, 2021 at 01:27:41AM CDT, Andrew Jeffery wrote:
Make the KCS device drivers responsible for allocating their own memory.
Until now the private data for the device driver was allocated internal
to the private data for the chardev interface. This coupling required
the slightly awkward API of passing through the struct size for the
driver private data to the chardev constructor, and then retrieving a
pointer to the driver private data from the allocated chardev memory.
In addition to being awkward, the arrangement prevents the
implementation of alternative userspace interfaces as the device driver
private data is not independent.
Peel a layer off the onion and turn the data-structures inside out by
exploiting container_of() and embedding `struct kcs_device` in the
driver private data.
Signed-off-by: Andrew Jeffery
---
drivers/char/ipmi/kcs_bmc.c | 15 +--
drivers/char/ipmi/kcs_bmc.h | 12 ++
drivers/char/ipmi/kcs_bmc_aspeed.c| 60 ---
drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 60 ++-
drivers/char/ipmi/kcs_bmc_npcm7xx.c | 37 ++---
5 files changed, 113 insertions(+), 71 deletions(-)
diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
index ef5c48ffe74a..709b6bdec165 100644
--- a/drivers/char/ipmi/kcs_bmc.c
+++ b/drivers/char/ipmi/kcs_bmc.c
@@ -44,12 +44,19 @@ int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc)
}
EXPORT_SYMBOL(kcs_bmc_handle_event);
-struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv,
u32 channel);
-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32
channel)
+int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc *kcs_bmc);
>>>
>>>Another declaration perhaps intended for kcs_bmc.h?
>>
>>These are temporary while the code gets shuffled around. The symbol
>>name is an implementation detail, not a "public" part of the API; after
>>some further shuffling these are eventually assigned as callbacks in an
>>ops struct.
>>
>
>Okay, that makes sense.
>
>>>
+int kcs_bmc_add_device(struct kcs_bmc *kcs_bmc)
{
- return kcs_bmc_ipmi_alloc(dev, sizeof_priv, channel);
+ return kcs_bmc_ipmi_attach_cdev(kcs_bmc);
}
-EXPORT_SYMBOL(kcs_bmc_alloc);
+EXPORT_SYMBOL(kcs_bmc_add_device);
+
+int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc *kcs_bmc);
>>>
>>>Here too.
>>>
+int kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc)
+{
+ return kcs_bmc_ipmi_detach_cdev(kcs_bmc);
+}
+EXPORT_SYMBOL(kcs_bmc_remove_device);
MODULE_LICENSE("GPL v2");
MODULE_AUTHOR("Haiyue Wang ");
diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h
index febea0c8deb4..bf0ae327997f 100644
--- a/drivers/char/ipmi/kcs_bmc.h
+++ b/drivers/char/ipmi/kcs_bmc.h
@@ -67,6 +67,8 @@ struct kcs_ioreg {
};
struct kcs_bmc {
+ struct device *dev;
+
spinlock_t lock;
u32 channel;
@@ -94,17 +96,11 @@ struct kcs_bmc {
u8 *kbuffer;
struct miscdevice miscdev;
-
- unsigned long priv[];
};
-static inline void *kcs_bmc_priv(struct kcs_bmc *kcs_bmc)
-{
- return kcs_bmc->priv;
-}
-
int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc);
-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32
channel);
+int kcs_bmc_add_device(struct kcs_bmc *kcs_bmc);
+int kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc);
u8 kcs_bmc_read_data(struct kcs_bmc *kcs_bmc);
void kcs_bmc_write_data(struct kcs_bmc *kcs_bmc, u8 data);
diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c
b/drivers/char/ipmi/kcs_bmc_aspeed.c
index 630cf095560e..0416ac78ce68 100644
--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
@@ -61,6 +61,8 @@
#define LPC_STR4 0x11C
struct aspeed_kcs_bmc {
+ struct kcs_bmc kcs_bmc;
+
struct regmap *map;
};
@@ -69,9 +71,14 @@ struct aspeed_kcs_of_ops {
int (*get_io_address)(struct platform_device *pdev);
};
+static inline struct aspeed_kcs_bmc *to_aspeed_kcs_bmc(struct kcs_bmc
*kcs_bmc)
+{
+ return container_of(kcs_bmc, struct aspeed_kcs_bmc, kcs_bmc);
+}
+
static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
{
- struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
+ struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
u32 val = 0;
int rc;
@@ -83,7 +90,7 @@ static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
Re: [PATCH v2 10/21] ipmi: kcs_bmc: Turn the driver data-structures inside-out
On Fri, Apr 09, 2021 at 12:59:09AM CDT, Andrew Jeffery wrote:
>
>
>On Fri, 9 Apr 2021, at 13:27, Zev Weiss wrote:
>> On Fri, Mar 19, 2021 at 01:27:41AM CDT, Andrew Jeffery wrote:
>> >Make the KCS device drivers responsible for allocating their own memory.
>> >
>> >Until now the private data for the device driver was allocated internal
>> >to the private data for the chardev interface. This coupling required
>> >the slightly awkward API of passing through the struct size for the
>> >driver private data to the chardev constructor, and then retrieving a
>> >pointer to the driver private data from the allocated chardev memory.
>> >
>> >In addition to being awkward, the arrangement prevents the
>> >implementation of alternative userspace interfaces as the device driver
>> >private data is not independent.
>> >
>> >Peel a layer off the onion and turn the data-structures inside out by
>> >exploiting container_of() and embedding `struct kcs_device` in the
>> >driver private data.
>> >
>> >Signed-off-by: Andrew Jeffery
>> >---
>> > drivers/char/ipmi/kcs_bmc.c | 15 +--
>> > drivers/char/ipmi/kcs_bmc.h | 12 ++
>> > drivers/char/ipmi/kcs_bmc_aspeed.c| 60 ---
>> > drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 60 ++-
>> > drivers/char/ipmi/kcs_bmc_npcm7xx.c | 37 ++---
>> > 5 files changed, 113 insertions(+), 71 deletions(-)
>> >
>> >diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
>> >index ef5c48ffe74a..709b6bdec165 100644
>> >--- a/drivers/char/ipmi/kcs_bmc.c
>> >+++ b/drivers/char/ipmi/kcs_bmc.c
>> >@@ -44,12 +44,19 @@ int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc)
>> > }
>> > EXPORT_SYMBOL(kcs_bmc_handle_event);
>> >
>> >-struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv,
>> >u32 channel);
>> >-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32
>> >channel)
>> >+int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc *kcs_bmc);
>>
>> Another declaration perhaps intended for kcs_bmc.h?
>
>These are temporary while the code gets shuffled around. The symbol
>name is an implementation detail, not a "public" part of the API; after
>some further shuffling these are eventually assigned as callbacks in an
>ops struct.
>
Okay, that makes sense.
>>
>> >+int kcs_bmc_add_device(struct kcs_bmc *kcs_bmc)
>> > {
>> >- return kcs_bmc_ipmi_alloc(dev, sizeof_priv, channel);
>> >+ return kcs_bmc_ipmi_attach_cdev(kcs_bmc);
>> > }
>> >-EXPORT_SYMBOL(kcs_bmc_alloc);
>> >+EXPORT_SYMBOL(kcs_bmc_add_device);
>> >+
>> >+int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc *kcs_bmc);
>>
>> Here too.
>>
>> >+int kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc)
>> >+{
>> >+ return kcs_bmc_ipmi_detach_cdev(kcs_bmc);
>> >+}
>> >+EXPORT_SYMBOL(kcs_bmc_remove_device);
>> >
>> > MODULE_LICENSE("GPL v2");
>> > MODULE_AUTHOR("Haiyue Wang ");
>> >diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h
>> >index febea0c8deb4..bf0ae327997f 100644
>> >--- a/drivers/char/ipmi/kcs_bmc.h
>> >+++ b/drivers/char/ipmi/kcs_bmc.h
>> >@@ -67,6 +67,8 @@ struct kcs_ioreg {
>> > };
>> >
>> > struct kcs_bmc {
>> >+ struct device *dev;
>> >+
>> >spinlock_t lock;
>> >
>> >u32 channel;
>> >@@ -94,17 +96,11 @@ struct kcs_bmc {
>> >u8 *kbuffer;
>> >
>> >struct miscdevice miscdev;
>> >-
>> >- unsigned long priv[];
>> > };
>> >
>> >-static inline void *kcs_bmc_priv(struct kcs_bmc *kcs_bmc)
>> >-{
>> >- return kcs_bmc->priv;
>> >-}
>> >-
>> > int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc);
>> >-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32
>> >channel);
>> >+int kcs_bmc_add_device(struct kcs_bmc *kcs_bmc);
>> >+int kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc);
>> >
>> > u8 kcs_bmc_read_data(struct kcs_bmc *kcs_bmc);
>> > void kcs_bmc_write_data(struct kcs_bmc *kcs_bmc, u8 data);
>> >diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c
>> >b/drivers/char/ipmi/kcs_bmc_aspeed.c
>> >index 630cf095560e..0416ac78ce68 100644
>> >--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
>> >+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
>> >@@ -61,6 +61,8 @@
>> > #define LPC_STR4 0x11C
>> >
>> > struct aspeed_kcs_bmc {
>> >+ struct kcs_bmc kcs_bmc;
>> >+
>> >struct regmap *map;
>> > };
>> >
>> >@@ -69,9 +71,14 @@ struct aspeed_kcs_of_ops {
>> >int (*get_io_address)(struct platform_device *pdev);
>> > };
>> >
>> >+static inline struct aspeed_kcs_bmc *to_aspeed_kcs_bmc(struct kcs_bmc
>> >*kcs_bmc)
>> >+{
>> >+ return container_of(kcs_bmc, struct aspeed_kcs_bmc, kcs_bmc);
>> >+}
>> >+
>> > static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
>> > {
>> >- struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
>> >+ struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
>> >u32 val = 0;
>> >int rc;
>> >
>> >@@ -83,7 +90,7 @@ static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
>> >
>> > static void aspeed_kcs_outb(struct kcs_bmc *kcs_bmc, u32 reg, u8 data)
>>
Re: [PATCH v2 10/21] ipmi: kcs_bmc: Turn the driver data-structures inside-out
On Fri, 9 Apr 2021, at 13:27, Zev Weiss wrote:
> On Fri, Mar 19, 2021 at 01:27:41AM CDT, Andrew Jeffery wrote:
> >Make the KCS device drivers responsible for allocating their own memory.
> >
> >Until now the private data for the device driver was allocated internal
> >to the private data for the chardev interface. This coupling required
> >the slightly awkward API of passing through the struct size for the
> >driver private data to the chardev constructor, and then retrieving a
> >pointer to the driver private data from the allocated chardev memory.
> >
> >In addition to being awkward, the arrangement prevents the
> >implementation of alternative userspace interfaces as the device driver
> >private data is not independent.
> >
> >Peel a layer off the onion and turn the data-structures inside out by
> >exploiting container_of() and embedding `struct kcs_device` in the
> >driver private data.
> >
> >Signed-off-by: Andrew Jeffery
> >---
> > drivers/char/ipmi/kcs_bmc.c | 15 +--
> > drivers/char/ipmi/kcs_bmc.h | 12 ++
> > drivers/char/ipmi/kcs_bmc_aspeed.c| 60 ---
> > drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 60 ++-
> > drivers/char/ipmi/kcs_bmc_npcm7xx.c | 37 ++---
> > 5 files changed, 113 insertions(+), 71 deletions(-)
> >
> >diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
> >index ef5c48ffe74a..709b6bdec165 100644
> >--- a/drivers/char/ipmi/kcs_bmc.c
> >+++ b/drivers/char/ipmi/kcs_bmc.c
> >@@ -44,12 +44,19 @@ int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc)
> > }
> > EXPORT_SYMBOL(kcs_bmc_handle_event);
> >
> >-struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv, u32
> >channel);
> >-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32
> >channel)
> >+int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc *kcs_bmc);
>
> Another declaration perhaps intended for kcs_bmc.h?
These are temporary while the code gets shuffled around. The symbol
name is an implementation detail, not a "public" part of the API; after
some further shuffling these are eventually assigned as callbacks in an
ops struct.
>
> >+int kcs_bmc_add_device(struct kcs_bmc *kcs_bmc)
> > {
> >-return kcs_bmc_ipmi_alloc(dev, sizeof_priv, channel);
> >+return kcs_bmc_ipmi_attach_cdev(kcs_bmc);
> > }
> >-EXPORT_SYMBOL(kcs_bmc_alloc);
> >+EXPORT_SYMBOL(kcs_bmc_add_device);
> >+
> >+int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc *kcs_bmc);
>
> Here too.
>
> >+int kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc)
> >+{
> >+return kcs_bmc_ipmi_detach_cdev(kcs_bmc);
> >+}
> >+EXPORT_SYMBOL(kcs_bmc_remove_device);
> >
> > MODULE_LICENSE("GPL v2");
> > MODULE_AUTHOR("Haiyue Wang ");
> >diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h
> >index febea0c8deb4..bf0ae327997f 100644
> >--- a/drivers/char/ipmi/kcs_bmc.h
> >+++ b/drivers/char/ipmi/kcs_bmc.h
> >@@ -67,6 +67,8 @@ struct kcs_ioreg {
> > };
> >
> > struct kcs_bmc {
> >+struct device *dev;
> >+
> > spinlock_t lock;
> >
> > u32 channel;
> >@@ -94,17 +96,11 @@ struct kcs_bmc {
> > u8 *kbuffer;
> >
> > struct miscdevice miscdev;
> >-
> >-unsigned long priv[];
> > };
> >
> >-static inline void *kcs_bmc_priv(struct kcs_bmc *kcs_bmc)
> >-{
> >-return kcs_bmc->priv;
> >-}
> >-
> > int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc);
> >-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32
> >channel);
> >+int kcs_bmc_add_device(struct kcs_bmc *kcs_bmc);
> >+int kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc);
> >
> > u8 kcs_bmc_read_data(struct kcs_bmc *kcs_bmc);
> > void kcs_bmc_write_data(struct kcs_bmc *kcs_bmc, u8 data);
> >diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c
> >b/drivers/char/ipmi/kcs_bmc_aspeed.c
> >index 630cf095560e..0416ac78ce68 100644
> >--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
> >+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
> >@@ -61,6 +61,8 @@
> > #define LPC_STR4 0x11C
> >
> > struct aspeed_kcs_bmc {
> >+struct kcs_bmc kcs_bmc;
> >+
> > struct regmap *map;
> > };
> >
> >@@ -69,9 +71,14 @@ struct aspeed_kcs_of_ops {
> > int (*get_io_address)(struct platform_device *pdev);
> > };
> >
> >+static inline struct aspeed_kcs_bmc *to_aspeed_kcs_bmc(struct kcs_bmc
> >*kcs_bmc)
> >+{
> >+return container_of(kcs_bmc, struct aspeed_kcs_bmc, kcs_bmc);
> >+}
> >+
> > static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
> > {
> >-struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
> >+struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
> > u32 val = 0;
> > int rc;
> >
> >@@ -83,7 +90,7 @@ static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
> >
> > static void aspeed_kcs_outb(struct kcs_bmc *kcs_bmc, u32 reg, u8 data)
> > {
> >-struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
> >+struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
> > int rc;
> >
> > rc = regmap_write(priv->map, reg, data);
>
Re: [PATCH v2 10/21] ipmi: kcs_bmc: Turn the driver data-structures inside-out
On Fri, Mar 19, 2021 at 01:27:41AM CDT, Andrew Jeffery wrote:
>Make the KCS device drivers responsible for allocating their own memory.
>
>Until now the private data for the device driver was allocated internal
>to the private data for the chardev interface. This coupling required
>the slightly awkward API of passing through the struct size for the
>driver private data to the chardev constructor, and then retrieving a
>pointer to the driver private data from the allocated chardev memory.
>
>In addition to being awkward, the arrangement prevents the
>implementation of alternative userspace interfaces as the device driver
>private data is not independent.
>
>Peel a layer off the onion and turn the data-structures inside out by
>exploiting container_of() and embedding `struct kcs_device` in the
>driver private data.
>
>Signed-off-by: Andrew Jeffery
>---
> drivers/char/ipmi/kcs_bmc.c | 15 +--
> drivers/char/ipmi/kcs_bmc.h | 12 ++
> drivers/char/ipmi/kcs_bmc_aspeed.c| 60 ---
> drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 60 ++-
> drivers/char/ipmi/kcs_bmc_npcm7xx.c | 37 ++---
> 5 files changed, 113 insertions(+), 71 deletions(-)
>
>diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
>index ef5c48ffe74a..709b6bdec165 100644
>--- a/drivers/char/ipmi/kcs_bmc.c
>+++ b/drivers/char/ipmi/kcs_bmc.c
>@@ -44,12 +44,19 @@ int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc)
> }
> EXPORT_SYMBOL(kcs_bmc_handle_event);
>
>-struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv, u32
>channel);
>-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32
>channel)
>+int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc *kcs_bmc);
Another declaration perhaps intended for kcs_bmc.h?
>+int kcs_bmc_add_device(struct kcs_bmc *kcs_bmc)
> {
>- return kcs_bmc_ipmi_alloc(dev, sizeof_priv, channel);
>+ return kcs_bmc_ipmi_attach_cdev(kcs_bmc);
> }
>-EXPORT_SYMBOL(kcs_bmc_alloc);
>+EXPORT_SYMBOL(kcs_bmc_add_device);
>+
>+int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc *kcs_bmc);
Here too.
>+int kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc)
>+{
>+ return kcs_bmc_ipmi_detach_cdev(kcs_bmc);
>+}
>+EXPORT_SYMBOL(kcs_bmc_remove_device);
>
> MODULE_LICENSE("GPL v2");
> MODULE_AUTHOR("Haiyue Wang ");
>diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h
>index febea0c8deb4..bf0ae327997f 100644
>--- a/drivers/char/ipmi/kcs_bmc.h
>+++ b/drivers/char/ipmi/kcs_bmc.h
>@@ -67,6 +67,8 @@ struct kcs_ioreg {
> };
>
> struct kcs_bmc {
>+ struct device *dev;
>+
> spinlock_t lock;
>
> u32 channel;
>@@ -94,17 +96,11 @@ struct kcs_bmc {
> u8 *kbuffer;
>
> struct miscdevice miscdev;
>-
>- unsigned long priv[];
> };
>
>-static inline void *kcs_bmc_priv(struct kcs_bmc *kcs_bmc)
>-{
>- return kcs_bmc->priv;
>-}
>-
> int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc);
>-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32
>channel);
>+int kcs_bmc_add_device(struct kcs_bmc *kcs_bmc);
>+int kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc);
>
> u8 kcs_bmc_read_data(struct kcs_bmc *kcs_bmc);
> void kcs_bmc_write_data(struct kcs_bmc *kcs_bmc, u8 data);
>diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c
>b/drivers/char/ipmi/kcs_bmc_aspeed.c
>index 630cf095560e..0416ac78ce68 100644
>--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
>+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
>@@ -61,6 +61,8 @@
> #define LPC_STR4 0x11C
>
> struct aspeed_kcs_bmc {
>+ struct kcs_bmc kcs_bmc;
>+
> struct regmap *map;
> };
>
>@@ -69,9 +71,14 @@ struct aspeed_kcs_of_ops {
> int (*get_io_address)(struct platform_device *pdev);
> };
>
>+static inline struct aspeed_kcs_bmc *to_aspeed_kcs_bmc(struct kcs_bmc
>*kcs_bmc)
>+{
>+ return container_of(kcs_bmc, struct aspeed_kcs_bmc, kcs_bmc);
>+}
>+
> static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
> {
>- struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
>+ struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
> u32 val = 0;
> int rc;
>
>@@ -83,7 +90,7 @@ static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
>
> static void aspeed_kcs_outb(struct kcs_bmc *kcs_bmc, u32 reg, u8 data)
> {
>- struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
>+ struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
> int rc;
>
> rc = regmap_write(priv->map, reg, data);
>@@ -92,7 +99,7 @@ static void aspeed_kcs_outb(struct kcs_bmc *kcs_bmc, u32
>reg, u8 data)
>
> static void aspeed_kcs_updateb(struct kcs_bmc *kcs_bmc, u32 reg, u8 mask, u8
> val)
> {
>- struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
>+ struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
> int rc;
>
> rc = regmap_update_bits(priv->map, reg, mask, val);
>@@ -114,7 +121,7 @@ static void aspeed_kcs_updateb(struct kcs_bmc *kcs_bmc,
>u32 reg, u8 mask, u8 val
> */
> static
[PATCH v2 10/21] ipmi: kcs_bmc: Turn the driver data-structures inside-out
Make the KCS device drivers responsible for allocating their own memory.
Until now the private data for the device driver was allocated internal
to the private data for the chardev interface. This coupling required
the slightly awkward API of passing through the struct size for the
driver private data to the chardev constructor, and then retrieving a
pointer to the driver private data from the allocated chardev memory.
In addition to being awkward, the arrangement prevents the
implementation of alternative userspace interfaces as the device driver
private data is not independent.
Peel a layer off the onion and turn the data-structures inside out by
exploiting container_of() and embedding `struct kcs_device` in the
driver private data.
Signed-off-by: Andrew Jeffery
---
drivers/char/ipmi/kcs_bmc.c | 15 +--
drivers/char/ipmi/kcs_bmc.h | 12 ++
drivers/char/ipmi/kcs_bmc_aspeed.c| 60 ---
drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 60 ++-
drivers/char/ipmi/kcs_bmc_npcm7xx.c | 37 ++---
5 files changed, 113 insertions(+), 71 deletions(-)
diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
index ef5c48ffe74a..709b6bdec165 100644
--- a/drivers/char/ipmi/kcs_bmc.c
+++ b/drivers/char/ipmi/kcs_bmc.c
@@ -44,12 +44,19 @@ int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc)
}
EXPORT_SYMBOL(kcs_bmc_handle_event);
-struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv, u32
channel);
-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32 channel)
+int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc *kcs_bmc);
+int kcs_bmc_add_device(struct kcs_bmc *kcs_bmc)
{
- return kcs_bmc_ipmi_alloc(dev, sizeof_priv, channel);
+ return kcs_bmc_ipmi_attach_cdev(kcs_bmc);
}
-EXPORT_SYMBOL(kcs_bmc_alloc);
+EXPORT_SYMBOL(kcs_bmc_add_device);
+
+int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc *kcs_bmc);
+int kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc)
+{
+ return kcs_bmc_ipmi_detach_cdev(kcs_bmc);
+}
+EXPORT_SYMBOL(kcs_bmc_remove_device);
MODULE_LICENSE("GPL v2");
MODULE_AUTHOR("Haiyue Wang ");
diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h
index febea0c8deb4..bf0ae327997f 100644
--- a/drivers/char/ipmi/kcs_bmc.h
+++ b/drivers/char/ipmi/kcs_bmc.h
@@ -67,6 +67,8 @@ struct kcs_ioreg {
};
struct kcs_bmc {
+ struct device *dev;
+
spinlock_t lock;
u32 channel;
@@ -94,17 +96,11 @@ struct kcs_bmc {
u8 *kbuffer;
struct miscdevice miscdev;
-
- unsigned long priv[];
};
-static inline void *kcs_bmc_priv(struct kcs_bmc *kcs_bmc)
-{
- return kcs_bmc->priv;
-}
-
int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc);
-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32
channel);
+int kcs_bmc_add_device(struct kcs_bmc *kcs_bmc);
+int kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc);
u8 kcs_bmc_read_data(struct kcs_bmc *kcs_bmc);
void kcs_bmc_write_data(struct kcs_bmc *kcs_bmc, u8 data);
diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c
b/drivers/char/ipmi/kcs_bmc_aspeed.c
index 630cf095560e..0416ac78ce68 100644
--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
@@ -61,6 +61,8 @@
#define LPC_STR4 0x11C
struct aspeed_kcs_bmc {
+ struct kcs_bmc kcs_bmc;
+
struct regmap *map;
};
@@ -69,9 +71,14 @@ struct aspeed_kcs_of_ops {
int (*get_io_address)(struct platform_device *pdev);
};
+static inline struct aspeed_kcs_bmc *to_aspeed_kcs_bmc(struct kcs_bmc *kcs_bmc)
+{
+ return container_of(kcs_bmc, struct aspeed_kcs_bmc, kcs_bmc);
+}
+
static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
{
- struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
+ struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
u32 val = 0;
int rc;
@@ -83,7 +90,7 @@ static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
static void aspeed_kcs_outb(struct kcs_bmc *kcs_bmc, u32 reg, u8 data)
{
- struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
+ struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
int rc;
rc = regmap_write(priv->map, reg, data);
@@ -92,7 +99,7 @@ static void aspeed_kcs_outb(struct kcs_bmc *kcs_bmc, u32 reg,
u8 data)
static void aspeed_kcs_updateb(struct kcs_bmc *kcs_bmc, u32 reg, u8 mask, u8
val)
{
- struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
+ struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
int rc;
rc = regmap_update_bits(priv->map, reg, mask, val);
@@ -114,7 +121,7 @@ static void aspeed_kcs_updateb(struct kcs_bmc *kcs_bmc, u32
reg, u8 mask, u8 val
*/
static void aspeed_kcs_set_address(struct kcs_bmc *kcs_bmc, u16 addr)
{
- struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
+ struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
switch (kcs_bmc->channel) {
