Skip to content

F
FMC Bus
Commit b4ae7fe6 authored by Alessandro Rubini's avatar Alessandro Rubini
Browse files
Options

fmc-core: bug fix requiring incompatible changes in registration 

My previous code for device management was wrong, because I didn't
understand the allocation. This fixes the thing, by properly using the
release function.

This change however requires that all fmc devices are allocated
separately (not as an array) and are not freed by the registering
entity.  So this introduces an incompatible change in the prototype
for register_n and unregister_n -- which is noted at build time, but
also an incompatible change in how a device must be allocated/freed
by the carrier that registers a single device. This change is
not introducing a build error, but I hope the only external users by now
are the SPEC and the SVEC.

fmc-fakedev is fixed by this same commit.
Signed-off-by: Alessandro Rubini's avatarAlessandro Rubini <rubini@gnudd.com>
parent f685f5b1
Hide whitespace changes
Inline Side-by-side
Showing with 78 additions and 88 deletions
doc/fmc-bus.in
View file @ b4ae7fe6
...@@ -161,8 +161,8 @@ to work, and a few more: ...@@ -161,8 +161,8 @@ to work, and a few more:
int fmc_device_register(struct fmc_device *fmc); int fmc_device_register(struct fmc_device *fmc);
void fmc_device_unregister(struct fmc_device *fmc); void fmc_device_unregister(struct fmc_device *fmc);
int fmc_device_register_n(struct fmc_device *fmc, int n); int fmc_device_register_n(struct fmc_device **fmc, int n);
void fmc_device_unregister_n(struct fmc_device *fmc, int n); void fmc_device_unregister_n(struct fmc_device **fmc, int n);
uint32_t fmc_readl(struct fmc_device *fmc, int offset); uint32_t fmc_readl(struct fmc_device *fmc, int offset);
void fmc_writel(struct fmc_device *fmc, uint32_t val, int off); void fmc_writel(struct fmc_device *fmc, uint32_t val, int off);
...@@ -175,11 +175,14 @@ to work, and a few more: ...@@ -175,11 +175,14 @@ to work, and a few more:
The data structure that describe a device is detailed in @ref{FMC The data structure that describe a device is detailed in @ref{FMC
Device}, the one that describes a driver is detailed in @ref{FMC Device}, the one that describes a driver is detailed in @ref{FMC
Driver}. Driver}. Please note that structures of type @t{fmc_device} must
be allocated by the caller, but must not be released after unregistering.
The @i{fmc-bus} itself takes care of releasing the structure when their
use count reaches zero -- actually, the device model does that in lieu of us.
The functions to register and unregister @i{n} devices are meant to be The functions to register and unregister @i{n} devices are meant to be
used by carriers that host more than one mezzanine. The devices must used by carriers that host more than one mezzanine. The devices must
be all registered at the same time because if the @sc{fpga} is all be registered at the same time because if the @sc{fpga} is
reprogrammed, all devices in the array are affected. Usually, the reprogrammed, all devices in the array are affected. Usually, the
driver matching the first device will reprogram the @sc{fpga}, so driver matching the first device will reprogram the @sc{fpga}, so
other devices must know they are already driven by a reprogrammed other devices must know they are already driven by a reprogrammed
...@@ -652,41 +655,23 @@ In order to register more than one mezzanine, you can use module parameters: ...@@ -652,41 +655,23 @@ In order to register more than one mezzanine, you can use module parameters:
@node FMC Device Incompatibilities @node FMC Device Incompatibilities
@section FMC Device Incompatibilities @section FMC Device Incompatibilities
If you started using this @i{fmc-bus} machinery some time ago, you may During development, we had to introduce a few incompatibilities in the
want to know what changed when the system has gained IPMI-FRU support. data structures. By using the field named @i{version} inside
If you are a new user, you can ignore this section. @t{fmc_device} you should be notified about such changes. Please use
@t{git blame} for the details -- the version is always updated in the
@table @code same commit that makes the change (for example commit 14a7b580 introduced
version 3, changed the data structures and documentation at the same time).
@item version See @ref{FMC Device} for the current list of fields.
The version number changed from 1 to 2, to prevent errors in those The other incompatible change we had to introduce is in the allocation
few users that (like me) don't run CONFIG_MODVERSIONS. Actually, policy for @t{fmc_device} structures. The devices must be allocated
no change is needed in driver source code. one at a time (with @i{kmalloc}) and never released. Releasing
happens by means of the @i{release} method of the device, when its
@item owner use count drops to zero.
The field is a new entry, and should be set to @code{THIS_MODULE}. As a side effect, @t{fmc_device_register_n} must be passed an array
The new ``generic'' driver, meant to finally obsolete the of pointers, not an array of devices. Since each device is freed
@i{gnurabbit} project, needs to pin a device while using it. separately when not in use any more.
@item memlen
In order to export a char device, we need to know the memory size,
so the device must tell how big it is (e.g., for the @sc{spec} it
is 1MB).
@item device_id
This is a new entry: earlier, some @sc{fmc} drivers had to
look at the PCI carrier-specific structures to build identifiers.
This should now be filled by the device before registering (if
missing, the core will use sequential numbers).
@end table
Other fields are new, but they are filled by the core and don't require
changes in driver modules.
@c ########################################################################## @c ##########################################################################
@node FMC Driver @node FMC Driver
......
kernel/fmc-core.c
View file @ b4ae7fe6
...@@ -67,8 +67,12 @@ static struct bus_type fmc_bus_type = { ...@@ -67,8 +67,12 @@ static struct bus_type fmc_bus_type = {
.shutdown = fmc_shutdown, .shutdown = fmc_shutdown,
}; };
/* We really have nothing to release in here */ static void fmc_release(struct device *dev)
static void __fmc_release(struct device *dev) { } {
struct fmc_device *fmc = container_of(dev, struct fmc_device, dev);
kfree(fmc);
}
/* /*
* Functions for client modules follow * Functions for client modules follow
...@@ -93,22 +97,26 @@ EXPORT_SYMBOL(fmc_driver_unregister); ...@@ -93,22 +97,26 @@ EXPORT_SYMBOL(fmc_driver_unregister);
* When a device set is registered, all eeproms must be read * When a device set is registered, all eeproms must be read
* and all FRUs must be parsed * and all FRUs must be parsed
*/ */
int fmc_device_register_n(struct fmc_device *fmcs, int n) int fmc_device_register_n(struct fmc_device **devs, int n)
{ {
struct fmc_device *fmc, **devarray; struct fmc_device *fmc, **devarray;
uint32_t device_id; uint32_t device_id;
int i, ret = 0; int i, ret = 0;
if (n < 1)
return 0;
/* Check the version of the first data structure (function prints) */ /* Check the version of the first data structure (function prints) */
if (fmc_check_version(fmcs->version, fmcs->carrier_name)) if (fmc_check_version(devs[0]->version, devs[0]->carrier_name))
return -EINVAL; return -EINVAL;
devarray = kmalloc(n * sizeof(*devarray), GFP_KERNEL); devarray = kmemdup(devs, n * sizeof(*devs), GFP_KERNEL);
if (!devarray) if (!devarray)
return -ENOMEM; return -ENOMEM;
/* Make all other checks before continuing, for all devices */ /* Make all other checks before continuing, for all devices */
for (i = 0, fmc = fmcs; i < n; i++, fmc++) { for (i = 0; i < n; i++) {
fmc = devarray[i];
if (!fmc->hwdev) { if (!fmc->hwdev) {
pr_err("%s: device has no hwdev pointer\n", __func__); pr_err("%s: device has no hwdev pointer\n", __func__);
return -EINVAL; return -EINVAL;
...@@ -130,8 +138,6 @@ int fmc_device_register_n(struct fmc_device *fmcs, int n) ...@@ -130,8 +138,6 @@ int fmc_device_register_n(struct fmc_device *fmcs, int n)
if (ret) if (ret)
break; break;
fmc->nr_slots = n; /* each slot must know how many are there */
devarray[i] = fmc;
} }
if (ret) { if (ret) {
kfree(devarray); kfree(devarray);
...@@ -139,15 +145,14 @@ int fmc_device_register_n(struct fmc_device *fmcs, int n) ...@@ -139,15 +145,14 @@ int fmc_device_register_n(struct fmc_device *fmcs, int n)
} }
/* Validation is ok. Now init and register the devices */ /* Validation is ok. Now init and register the devices */
for (i = 0, fmc = fmcs; i < n; i++, fmc++) { for (i = 0; i < n; i++) {
fmc = devarray[i];
fmc->nr_slots = n; /* each slot must know how many are there */
fmc->devarray = devarray; fmc->devarray = devarray;
device_initialize(&fmc->dev); device_initialize(&fmc->dev);
if (!fmc->dev.release) fmc->dev.release = fmc_release;
fmc->dev.release = __fmc_release; fmc->dev.parent = fmc->hwdev;
if (!fmc->dev.parent)
fmc->dev.parent = fmc->hwdev;
/* Fill the identification stuff (may fail) */ /* Fill the identification stuff (may fail) */
fmc_fill_id_info(fmc); fmc_fill_id_info(fmc);
...@@ -166,6 +171,7 @@ int fmc_device_register_n(struct fmc_device *fmcs, int n) ...@@ -166,6 +171,7 @@ int fmc_device_register_n(struct fmc_device *fmcs, int n)
dev_err(fmc->hwdev, "Failed in registering \"%s\"\n", dev_err(fmc->hwdev, "Failed in registering \"%s\"\n",
fmc->dev.kobj.name); fmc->dev.kobj.name);
fmc_free_id_info(fmc); fmc_free_id_info(fmc);
put_device(&fmc->dev);
goto out; goto out;
} }
/* This device went well, give information to the user */ /* This device went well, give information to the user */
...@@ -175,14 +181,12 @@ int fmc_device_register_n(struct fmc_device *fmcs, int n) ...@@ -175,14 +181,12 @@ int fmc_device_register_n(struct fmc_device *fmcs, int n)
return 0; return 0;
out: out:
for (i--, fmc--; i >= 0; i--, fmc--) { kfree(devarray);
device_del(&fmc->dev); for (i--; i >= 0; i--) {
fmc_free_id_info(fmc); device_del(&devs[i]->dev);
put_device(&fmc->dev); fmc_free_id_info(devs[i]);
put_device(&devs[i]->dev);
} }
kfree(fmcs->devarray);
for (i = 0, fmc = fmcs; i < n; i++, fmc++)
fmc->devarray = NULL;
return ret; return ret;
} }
...@@ -190,32 +194,31 @@ EXPORT_SYMBOL(fmc_device_register_n); ...@@ -190,32 +194,31 @@ EXPORT_SYMBOL(fmc_device_register_n);
int fmc_device_register(struct fmc_device *fmc) int fmc_device_register(struct fmc_device *fmc)
{ {
return fmc_device_register_n(fmc, 1); return fmc_device_register_n(&fmc, 1);
} }
EXPORT_SYMBOL(fmc_device_register); EXPORT_SYMBOL(fmc_device_register);
void fmc_device_unregister_n(struct fmc_device *fmcs, int n) void fmc_device_unregister_n(struct fmc_device **devs, int n)
{ {
struct fmc_device *fmc;
int i; int i;
for (i = 0, fmc = fmcs; i < n; i++, fmc++) { if (n < 1)
device_del(&fmc->dev); return;
fmc_free_id_info(fmc);
put_device(&fmc->dev); /* Free devarray first, not used by the later loop */
} kfree(devs[0]->devarray);
/* Then, free the locally-allocated stuff */
for (i = 0, fmc = fmcs; i < n; i++, fmc++) { for (i = 0; i < n; i++) {
if (i == 0) device_del(&devs[i]->dev);
kfree(fmc->devarray); fmc_free_id_info(devs[i]);
fmc->devarray = NULL; put_device(&devs[i]->dev);
} }
} }
EXPORT_SYMBOL(fmc_device_unregister_n); EXPORT_SYMBOL(fmc_device_unregister_n);
void fmc_device_unregister(struct fmc_device *fmc) void fmc_device_unregister(struct fmc_device *fmc)
{ {
fmc_device_unregister_n(fmc, 1); fmc_device_unregister_n(&fmc, 1);
} }
EXPORT_SYMBOL(fmc_device_unregister); EXPORT_SYMBOL(fmc_device_unregister);
......
kernel/fmc-fakedev.c
View file @ b4ae7fe6
...@@ -80,7 +80,7 @@ static char ff_eeimg[FF_MAX_MEZZANINES][FF_EEPROM_SIZE] = { ...@@ -80,7 +80,7 @@ static char ff_eeimg[FF_MAX_MEZZANINES][FF_EEPROM_SIZE] = {
}; };
struct ff_dev { struct ff_dev {
struct fmc_device fmc[FF_MAX_MEZZANINES]; struct fmc_device *fmc[FF_MAX_MEZZANINES];
struct device dev; struct device dev;
struct delayed_work work; struct delayed_work work;
}; };
...@@ -143,13 +143,13 @@ static void ff_work_fn(struct work_struct *work) ...@@ -143,13 +143,13 @@ static void ff_work_fn(struct work_struct *work)
ff = ff_dev_create(); ff = ff_dev_create();
if (IS_ERR(ff)) { if (IS_ERR(ff)) {
pr_warning("%s: can't re-create FMC device\n", __func__); pr_warning("%s: can't re-create FMC devices\n", __func__);
return; return;
} }
ret = fmc_device_register_n(ff->fmc, ff_nr_dev); ret = fmc_device_register_n(ff->fmc, ff_nr_dev);
if (ret < 0) { if (ret < 0) {
dev_warn(&ff->dev, "can't re-register FMC devices\n");
device_unregister(&ff->dev); device_unregister(&ff->dev);
pr_warning("%s: can't re-register FMC device\n", __func__);
return; return;
} }
...@@ -258,6 +258,7 @@ static struct fmc_device ff_template_fmc = { ...@@ -258,6 +258,7 @@ static struct fmc_device ff_template_fmc = {
static struct ff_dev *ff_dev_create(void) static struct ff_dev *ff_dev_create(void)
{ {
struct ff_dev *ff; struct ff_dev *ff;
struct fmc_device *fmc;
int i, ret; int i, ret;
ff = kzalloc(sizeof(*ff), GFP_KERNEL); ff = kzalloc(sizeof(*ff), GFP_KERNEL);
...@@ -269,20 +270,21 @@ static struct ff_dev *ff_dev_create(void) ...@@ -269,20 +270,21 @@ static struct ff_dev *ff_dev_create(void)
ret = device_register(&ff->dev); ret = device_register(&ff->dev);
if (ret < 0) { if (ret < 0) {
put_device(&ff->dev); put_device(&ff->dev);
kfree(ff);
return ERR_PTR(ret); return ERR_PTR(ret);
} }
/* Create fmc structures that refers to this new "hw" device */ /* Create fmc structures that refer to this new "hw" device */
for (i = 0; i < ff_nr_dev; i++) { for (i = 0; i < ff_nr_dev; i++) {
ff->fmc[i] = ff_template_fmc; fmc = kmemdup(&ff_template_fmc, sizeof(ff_template_fmc),
ff->fmc[i].hwdev = &ff->dev; GFP_KERNEL);
ff->fmc[i].carrier_data = ff; fmc->hwdev = &ff->dev;
ff->fmc[i].nr_slots = ff_nr_dev; fmc->carrier_data = ff;
fmc->nr_slots = ff_nr_dev;
/* the following fields are different for each slot */ /* the following fields are different for each slot */
ff->fmc[i].eeprom = ff_eeimg[i]; fmc->eeprom = ff_eeimg[i];
ff->fmc[i].eeprom_addr = 0x50 + 2 * i; fmc->eeprom_addr = 0x50 + 2 * i;
ff->fmc[i].slot_id = i; fmc->slot_id = i;
ff->fmc[i] = fmc;
/* increment the identifier, each must be different */ /* increment the identifier, each must be different */
ff_template_fmc.device_id++; ff_template_fmc.device_id++;
} }
......
kernel/include/linux/fmc.h
View file @ b4ae7fe6
...@@ -224,8 +224,8 @@ extern int fmc_device_register(struct fmc_device *tdev); ...@@ -224,8 +224,8 @@ extern int fmc_device_register(struct fmc_device *tdev);
extern void fmc_device_unregister(struct fmc_device *tdev); extern void fmc_device_unregister(struct fmc_device *tdev);
/* Two more for device sets, all driven by the same FPGA */ /* Two more for device sets, all driven by the same FPGA */
extern int fmc_device_register_n(struct fmc_device *fmc, int n); extern int fmc_device_register_n(struct fmc_device **devs, int n);
extern void fmc_device_unregister_n(struct fmc_device *fmc, int n); extern void fmc_device_unregister_n(struct fmc_device **devs, int n);
/* Internal cross-calls between files; not exported to other modules */ /* Internal cross-calls between files; not exported to other modules */
extern int fmc_match(struct device *dev, struct device_driver *drv); extern int fmc_match(struct device *dev, struct device_driver *drv);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment