| Message ID | 201801242122.31033.pisa@cmp.felk.cvut.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Pavel, On 01/24/2018 05:22 PM, Pavel Pisa wrote: > Hello everybody, > > On Tuesday 23 of January 2018 22:42:31 Pavel Pisa wrote: >> When Linux specific object file is linked in then some local >> function needs to be called before QOM instances population. >> I know how to do that GCC specific/non-portable way >> >> static void __attribute__((constructor)) can_socketcan_setup_variant(void) >> { >> >> } >> >> but I expect that something like >> >> module_init() >> >> in can_socketcan.c should be used. > > > I have experimented with code changes to get rid of stub for > non Linux systems. type_init() is used because it is more > portable than constructor attribute. > > I have seen that a few other type_init-s do more > than simple sequence of type_register_static(). > Is it acceptable to use type_init for registration > to CAN core by function call for now? Conversion simplifies > makefiles and unnecessary stub file is removed. > > But I would use attribute if that solution is preferred because > it is allways present on Linux where SocketCAN is used anyway > and it is used in other Qemu subsystems as well. using stubs/monitor.c as a template for stubs/can_host_variant.c doesn't work? > > ---------------------------------------------------------------- > Solution with attribute > > #ifdef TOOLCHAIN_SUPPORTS_ATTRIBUTE_CONSTRUCTOR > static void __attribute__((constructor)) > can_bus_socketcan_setup(void) > { > can_bus_set_connect_to_host(can_bus_connect_to_host_socketcan); > } > #endif > > ---------------------------------------------------------------- > Solution with type_init > branch https://gitlab.fel.cvut.cz/canbus/qemu-canbus/tree/can-pci-socketcan-init > > diff --git a/hw/can/can_socketcan.c b/hw/can/can_socketcan.c > index 42099fb696..fb41853c2b 100644 > --- a/hw/can/can_socketcan.c > +++ b/hw/can/can_socketcan.c > @@ -309,5 +309,14 @@ static int can_bus_connect_to_host_socketcan(CanBusState *bus, > return 0; > } > > -int (*can_bus_connect_to_host_variant)(CanBusState *bus, const char *name) = > - can_bus_connect_to_host_socketcan; > +static void can_bus_socketcan_type_init(void) > +{ > + /* > + * There should be object registration when CanBusSocketcanConnectState > + * is converted into QOM object. Use for registration of host > + * can bus access for now. > + */ > + can_bus_set_connect_to_host(can_bus_connect_to_host_socketcan); > +} > + > +type_init(can_bus_socketcan_type_init); > > > diff --git a/hw/can/Makefile.objs b/hw/can/Makefile.objs > index 1ce9950de0..14c2369718 100644 > --- a/hw/can/Makefile.objs > +++ b/hw/can/Makefile.objs > @@ -2,11 +2,7 @@ > > ifeq ($(CONFIG_CAN_BUS),y) > common-obj-y += can_core.o > -ifeq ($(CONFIG_LINUX),y) > -common-obj-y += can_socketcan.o > -else > -common-obj-y += can_host_stub.o > -endif > +common-obj-$(CONFIG_LINUX) += can_socketcan.o > common-obj-$(CONFIG_CAN_SJA1000) += can_sja1000.o > common-obj-$(CONFIG_CAN_PCI) += can_kvaser_pci.o > common-obj-$(CONFIG_CAN_PCI) += can_pcm3680_pci.o > diff --git a/hw/can/can_core.c b/hw/can/can_core.c > index 41c458c792..c14807b188 100644 > --- a/hw/can/can_core.c > +++ b/hw/can/can_core.c > @@ -34,6 +34,8 @@ > static QTAILQ_HEAD(, CanBusState) can_buses = > QTAILQ_HEAD_INITIALIZER(can_buses); > > +static can_bus_connect_to_host_t can_bus_connect_to_host_fnc; > + > CanBusState *can_bus_find_by_name(const char *name, bool create_missing) > { > CanBusState *bus; > @@ -127,10 +129,15 @@ int can_bus_client_set_filters(CanBusClientState *client, > > int can_bus_connect_to_host_device(CanBusState *bus, const char *name) > { > - if (can_bus_connect_to_host_variant == NULL) { > + if (can_bus_connect_to_host_fnc == NULL) { > error_report("CAN bus connect to host device is not " > "supported on this system"); > exit(1); > } > - return can_bus_connect_to_host_variant(bus, name); > + return can_bus_connect_to_host_fnc(bus, name); > +} > + > +void can_bus_set_connect_to_host(can_bus_connect_to_host_t connect_fnc) > +{ > + can_bus_connect_to_host_fnc = connect_fnc; > } > diff --git a/hw/can/can_host_stub.c b/hw/can/can_host_stub.c > deleted file mode 100644 > index 748d25f995..0000000000 > --- a/hw/can/can_host_stub.c > +++ /dev/null > @@ -1,36 +0,0 @@ > .... > .... > .... > - > - > -int (*can_bus_connect_to_host_variant)(CanBusState *bus, const char *name) = > - NULL; > > diff --git a/include/can/can_emu.h b/include/can/can_emu.h > index 85237ee3c9..7f0705e49f 100644 > --- a/include/can/can_emu.h > +++ b/include/can/can_emu.h > @@ -107,8 +107,9 @@ struct CanBusState { > QTAILQ_ENTRY(CanBusState) next; > }; > > -extern int (*can_bus_connect_to_host_variant)(CanBusState *bus, > - const char *name); > +typedef int (*can_bus_connect_to_host_t)(CanBusState *bus, const char *name); > + > +void can_bus_set_connect_to_host(can_bus_connect_to_host_t connect_fnc); > > int can_bus_filter_match(struct qemu_can_filter *filter, qemu_canid_t can_id); > ---------------------------------------------------------------- > > Best wishes, > > Pavel >
Hello Philippe, On Wednesday 24 of January 2018 22:41:16 Philippe Mathieu-Daudé wrote: > Hi Pavel, > > I have seen that a few other type_init-s do more > > than simple sequence of type_register_static(). > > Is it acceptable to use type_init for registration > > to CAN core by function call for now? Conversion simplifies > > makefiles and unnecessary stub file is removed. > > > > But I would use attribute if that solution is preferred because > > it is allways present on Linux where SocketCAN is used anyway > > and it is used in other Qemu subsystems as well. > > using stubs/monitor.c as a template for stubs/can_host_variant.c doesn't > work? If that is preferred then I implement the stub this way. As for the location, can I add stub-obj-y += can_host_stub.o into hw/can/Makefile.objs same as it is in crypto/Makefile.objs to keep CAN stuff together at least for now or it should go to stubs directory? stub-obj-$(CONFIG_CAN_BUS) += can_host_variant.o As for connection to host, again I have weak preference to keep it in hw/can to keep CAN related code together but if you think that it should go t chardev, I would prepare patch that way chardev/can-socketcan.c As for the rest of the remarks You are right that there is some code duplication in the SJA1000 CAN PCI cards support but problem is that KVASER single uses S5920 PCI local bus bridge which requires additional BARs and additional bridge specific interrupt enable support. There are more KVASER variants which combine multiple SJA1000 in a single BAR. pcm3680 and mioe3680 have different BAR structure, each SJA1000 uses one BAR. The first uses I/O mapped SJA1000 and another memory mapped with stride 4. Yes, it all can be combined into one C file. But it would require to to add more more fields into CardX_PCIState structure and some mechanisms and code to select right combination of the BARs, handlers etc for each card. It all can be done, but I am not sure if I find time for such changes now. I expect to have time again in summer when my teaching semester ends. Another disadvantage is that if somebody else wants to implement other card emulation then actual simple can_kvaser_pci.c is easily readable. Code gets much more complex with all variants selection logic and we have already abandoned that simple PCI example (can_pci.c) with dummy PCI ID which as been included in the past. If the code duplication is a problem for now then I vote to include only can_kvaser_pci.c for now. But Deniz Eren would be sad because he uses the cards (which he has contributed) in his test environment. Anyway, I would follow what is proposed. Thanks for your review and time, Pavel
Hi Pavel, Philippe, I’m happy with whatever way is best for the project. However I would personally think merging the different drivers into one C file would not be a very modular way of handling the problem. As you can see from the Advantech drivers for example, the card supplier end can pose difficult Bar allocation logic, which I would think is best isolated within the particular driver model C file. Trying to come up with mechanisms to handle these could prove difficult. Also keep in mind there should be more driver support added in future, which could pose other driver model specific difficulties that are best isolated to their own C files. Having said all that, there might be a nice way of merging them all generically which I fail to see currently. Best regards, Deniz Sent from my iPhone Deniz Eren +61 400 307 762 > On 25 Jan 2018, at 7:24 pm, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote: > > Hello Philippe, > >> On Wednesday 24 of January 2018 22:41:16 Philippe Mathieu-Daudé wrote: >> Hi Pavel, >>> I have seen that a few other type_init-s do more >>> than simple sequence of type_register_static(). >>> Is it acceptable to use type_init for registration >>> to CAN core by function call for now? Conversion simplifies >>> makefiles and unnecessary stub file is removed. >>> >>> But I would use attribute if that solution is preferred because >>> it is allways present on Linux where SocketCAN is used anyway >>> and it is used in other Qemu subsystems as well. >> >> using stubs/monitor.c as a template for stubs/can_host_variant.c doesn't >> work? > > If that is preferred then I implement the stub this way. > As for the location, can I add > > stub-obj-y += can_host_stub.o > > into hw/can/Makefile.objs same as it is in crypto/Makefile.objs > to keep CAN stuff together at least for now or it should go > to stubs directory? > > stub-obj-$(CONFIG_CAN_BUS) += can_host_variant.o > > As for connection to host, again I have weak preference > to keep it in hw/can to keep CAN related code together > but if you think that it should go t chardev, I would prepare > patch that way > > chardev/can-socketcan.c > > As for the rest of the remarks > > You are right that there is some code duplication > in the SJA1000 CAN PCI cards support but problem > is that KVASER single uses S5920 PCI local bus bridge > which requires additional BARs and additional bridge > specific interrupt enable support. There are more KVASER > variants which combine multiple SJA1000 in a single BAR. > pcm3680 and mioe3680 have different BAR structure, > each SJA1000 uses one BAR. The first uses I/O mapped > SJA1000 and another memory mapped with stride 4. > Yes, it all can be combined into one C file. > But it would require to to add more more fields > into CardX_PCIState structure and some mechanisms > and code to select right combination of the BARs, > handlers etc for each card. It all can be done, > but I am not sure if I find time for such changes now. > I expect to have time again in summer when my teaching > semester ends. > Another disadvantage is that if somebody else wants > to implement other card emulation then actual simple > can_kvaser_pci.c is easily readable. Code gets much > more complex with all variants selection logic and > we have already abandoned that simple PCI example > (can_pci.c) with dummy PCI ID which as been included > in the past. > > If the code duplication is a problem for now then > I vote to include only can_kvaser_pci.c for now. > But Deniz Eren would be sad because he uses the > cards (which he has contributed) in his test environment. > > Anyway, I would follow what is proposed. > > Thanks for your review and time, > > Pavel
diff --git a/hw/can/can_socketcan.c b/hw/can/can_socketcan.c index 42099fb696..fb41853c2b 100644 --- a/hw/can/can_socketcan.c +++ b/hw/can/can_socketcan.c @@ -309,5 +309,14 @@ static int can_bus_connect_to_host_socketcan(CanBusState *bus, return 0; } -int (*can_bus_connect_to_host_variant)(CanBusState *bus, const char *name) = - can_bus_connect_to_host_socketcan; +static void can_bus_socketcan_type_init(void) +{ + /* + * There should be object registration when CanBusSocketcanConnectState + * is converted into QOM object. Use for registration of host + * can bus access for now. + */ + can_bus_set_connect_to_host(can_bus_connect_to_host_socketcan); +} + +type_init(can_bus_socketcan_type_init); diff --git a/hw/can/Makefile.objs b/hw/can/Makefile.objs index 1ce9950de0..14c2369718 100644 --- a/hw/can/Makefile.objs +++ b/hw/can/Makefile.objs @@ -2,11 +2,7 @@ ifeq ($(CONFIG_CAN_BUS),y) common-obj-y += can_core.o -ifeq ($(CONFIG_LINUX),y) -common-obj-y += can_socketcan.o -else -common-obj-y += can_host_stub.o -endif +common-obj-$(CONFIG_LINUX) += can_socketcan.o common-obj-$(CONFIG_CAN_SJA1000) += can_sja1000.o common-obj-$(CONFIG_CAN_PCI) += can_kvaser_pci.o common-obj-$(CONFIG_CAN_PCI) += can_pcm3680_pci.o diff --git a/hw/can/can_core.c b/hw/can/can_core.c index 41c458c792..c14807b188 100644 --- a/hw/can/can_core.c +++ b/hw/can/can_core.c @@ -34,6 +34,8 @@ static QTAILQ_HEAD(, CanBusState) can_buses = QTAILQ_HEAD_INITIALIZER(can_buses); +static can_bus_connect_to_host_t can_bus_connect_to_host_fnc; + CanBusState *can_bus_find_by_name(const char *name, bool create_missing) { CanBusState *bus; @@ -127,10 +129,15 @@ int can_bus_client_set_filters(CanBusClientState *client, int can_bus_connect_to_host_device(CanBusState *bus, const char *name) { - if (can_bus_connect_to_host_variant == NULL) { + if (can_bus_connect_to_host_fnc == NULL) { error_report("CAN bus connect to host device is not " "supported on this system"); exit(1); } - return can_bus_connect_to_host_variant(bus, name); + return can_bus_connect_to_host_fnc(bus, name); +} + +void can_bus_set_connect_to_host(can_bus_connect_to_host_t connect_fnc) +{ + can_bus_connect_to_host_fnc = connect_fnc; } diff --git a/hw/can/can_host_stub.c b/hw/can/can_host_stub.c deleted file mode 100644 index 748d25f995..0000000000 --- a/hw/can/can_host_stub.c +++ /dev/null @@ -1,36 +0,0 @@ .... .... .... - - -int (*can_bus_connect_to_host_variant)(CanBusState *bus, const char *name) = - NULL; diff --git a/include/can/can_emu.h b/include/can/can_emu.h index 85237ee3c9..7f0705e49f 100644 --- a/include/can/can_emu.h +++ b/include/can/can_emu.h @@ -107,8 +107,9 @@ struct CanBusState { QTAILQ_ENTRY(CanBusState) next; }; -extern int (*can_bus_connect_to_host_variant)(CanBusState *bus, - const char *name); +typedef int (*can_bus_connect_to_host_t)(CanBusState *bus, const char *name); + +void can_bus_set_connect_to_host(can_bus_connect_to_host_t connect_fnc); int can_bus_filter_match(struct qemu_can_filter *filter, qemu_canid_t can_id); ---------------------------------------------------------------- Best wishes, Pavel