search

davidker / unisys

Master repository for new changes to drivers/staging/unisys and drivers/visorbus
Other
2 stars 1 forks source link

visorbus: a gazillion of exports which are just wrappers around another set of exports #59

Closed selltc closed 8 years ago

selltc commented 8 years ago

Source: Thomas Gleixner tglx@linutronix.de 5/18/2016:

A gazillion of exports which are just wrappers around another set of exports

See KanBoard-1251.

selltc commented 8 years ago

I THINK what tglx would prefer is that we use static inline functions in a header file rather than real exported functions. Here is the entire set of functions he is referring to, but my feeling is that only the very-trivial ones should be moved to static inline in a header file:

./visorbus/visorchannel.c:EXPORT_SYMBOL_GPL(visorchannel_create);
./visorbus/visorchannel.c:EXPORT_SYMBOL_GPL(visorchannel_create_with_lock);
./visorbus/visorchannel.c:EXPORT_SYMBOL_GPL(visorchannel_destroy);
./visorbus/visorchannel.c:EXPORT_SYMBOL_GPL(visorchannel_get_physaddr);
./visorbus/visorchannel.c:EXPORT_SYMBOL_GPL(visorchannel_get_nbytes);
./visorbus/visorchannel.c:EXPORT_SYMBOL_GPL(visorchannel_uuid_id);
./visorbus/visorchannel.c:EXPORT_SYMBOL_GPL(visorchannel_id);
./visorbus/visorchannel.c:EXPORT_SYMBOL_GPL(visorchannel_zoneid);
./visorbus/visorchannel.c:EXPORT_SYMBOL_GPL(visorchannel_get_clientpartition);
./visorbus/visorchannel.c:EXPORT_SYMBOL_GPL(visorchannel_set_clientpartition);
./visorbus/visorchannel.c:EXPORT_SYMBOL_GPL(visorchannel_get_uuid);
./visorbus/visorchannel.c:EXPORT_SYMBOL_GPL(visorchannel_read);
./visorbus/visorchannel.c:EXPORT_SYMBOL_GPL(visorchannel_write);
./visorbus/visorchannel.c:EXPORT_SYMBOL_GPL(visorchannel_clear);
./visorbus/visorchannel.c:EXPORT_SYMBOL_GPL(visorchannel_get_header);
./visorbus/visorchannel.c:EXPORT_SYMBOL_GPL(visorchannel_signalremove);
./visorbus/visorchannel.c:EXPORT_SYMBOL_GPL(visorchannel_signalempty);
./visorbus/visorchannel.c:EXPORT_SYMBOL_GPL(visorchannel_signalinsert);
./visorbus/visorchannel.c:EXPORT_SYMBOL_GPL(visorchannel_signalqueue_slots_avail);
./visorbus/visorchannel.c:EXPORT_SYMBOL_GPL(visorchannel_signalqueue_max_slots);
./visorbus/visorchannel.c:EXPORT_SYMBOL_GPL(visorchannel_debug);
./visorbus/visorchipset.c:EXPORT_SYMBOL(visorbus_get_device_by_id);
./visorbus/visorchipset.c:EXPORT_SYMBOL_GPL(visorchipset_register_busdev);
./visorbus/visorbus_main.c:EXPORT_SYMBOL_GPL(visorbus_register_visor_driver);
./visorbus/visorbus_main.c:EXPORT_SYMBOL_GPL(visorbus_unregister_visor_driver);
./visorbus/visorbus_main.c:EXPORT_SYMBOL_GPL(visorbus_read_channel);
./visorbus/visorbus_main.c:EXPORT_SYMBOL_GPL(visorbus_write_channel);
./visorbus/visorbus_main.c:EXPORT_SYMBOL_GPL(visorbus_clear_channel);
./visorbus/visorbus_main.c:EXPORT_SYMBOL_GPL(visorbus_enable_channel_interrupts);
./visorbus/visorbus_main.c:EXPORT_SYMBOL_GPL(visorbus_disable_channel_interrupts);
davidker commented 8 years ago

Is he also talking about visorbus_read_channel and visorbus_write_channel along with visorchannel_read and visorchannel_write? I've always look at those initially and get confused and then I start working in the code and start using them and forget about my confusion.

davidker commented 8 years ago

Should these not just be moved to the visorbus_private.h file and called from one function to another in the code? How much of these do we force our functional drivers to call directly. I think the export_symbol stuff is due to when visorchannel was its own driver. And should we have EXPORT_SYMBOL(visorbus_get_device_by_id); be EXPORT_SYMBOL_GPL as well?

selltc commented 8 years ago

Yes, visorbus_read_channel() and visorbus_write_channel() can either be converted to static inlines in a header file, or removed entirely. They are just "convenience functions".

I think our general strategy should just be to remove as many of the EXPORT_SYMBOLs as is practically possible. I don't think we need to remove them all.

I'm not sure what the rules are regarding EXPORT_SYMBOL_GPL. There are still kazillions of references to EXPORT_SYMBOL in the kernel.

selltc commented 8 years ago

https://lwn.net/Articles/603131/

https://lwn.net/Articles/603187/ suggests that Greg prefers EXPORT_SYMBOL_GPL.

This issue of EXPORT_SYMBOL_GPL or EXPORT_SYMBOL is whether proprietary binary kernel modules can call the function or not. Being proprietary, such modules will NOT have source code in the kernel tree. This is of course moot right now, since we have the only drivers that use the symbols, and they are NOT proprietary. It seems that changing to EXPORT_SYMBOL_GPL might be the safest thing.

davidker commented 8 years ago

Agreed, I thought they were all supposed to be switched. Thanks Tim for finding them! I'm also worried that we use more of this during for-later, but I guess we should relook into how we expose them for that branch as well.

mriswyth commented 8 years ago

I grabbed this from the Kanban board for now. I know there was discussion in the standup where Erik was asking about it and David said he would take it, but I couldn't tell if that was because Erik was so tentative about it.

@davidker if you really would rather just do this yourself just let me know.

mriswyth commented 8 years ago

I have committed the upstream-next version in the githubissue#59-upstream-next branch.

I separated the changes into 3 patches

I'm working on a for-later version of these changes now.

davidker commented 8 years ago

Nice! Did you see my advice on Tim on how to do the changes for the for-later branch?

mriswyth commented 8 years ago

@davidker I did see your advice which I never would have thought of, so thank you.

I have pushed a for-later version of the patches (in front of the "for-later" patches) in branch githubissue#59-for-later.

davidker commented 8 years ago

I made the comment on the patch but I'll make it here as well. I did notice this function is still declared in the .h should it be removed as well? And then it led me to ask what STANDALONE_CLIENT is and if it is even defined and if we should just get rid of it.

davidker commented 8 years ago

I also got compile errors when I compile with -Werror on.

make[1]: Entering directory/home/kershnda/kernel/master/unisys/.build' CHK include/config/kernel.release GEN ./Makefile CHK include/generated/uapi/linux/version.h Using .. as source for kernel CHK include/generated/utsrelease.h CHK include/generated/timeconst.h CHK include/generated/bounds.h CHK include/generated/asm-offsets.h CALL ../scripts/checksyscalls.sh CHK include/generated/compile.h CC drivers/staging/unisys/visorinput/visorinput.o CC drivers/staging/unisys/visorhba/visorhba_main.o CC drivers/staging/unisys/visorbus/visorbus_main.o CC drivers/staging/unisys/visornic/visornic_main.o CC drivers/staging/unisys/visorbus/visorchannel.o CC drivers/staging/unisys/visorbus/visorchipset.o ../drivers/staging/unisys/visorbus/visorchannel.c:460:1: error: 'sigqueue_debug' defined but not used [-Werror=unused-function] sigqueue_debug(struct signal_queue_header _q, int which, struct seqfile seq) ^ cc1: all warnings being treated as errors make[5]: * [drivers/staging/unisys/visorbus/visorchannel.o] Error 1 make[5]: *\ Waiting for unfinished jobs.... LD drivers/staging/unisys/visorinput/built-in.o LD drivers/staging/unisys/visornic/visornic.o `

selltc commented 8 years ago

STANDALONE_CLIENT was only used via the visorserial driver, which we don't even have in the upstream. So yeah, we can remove it.

selltc commented 8 years ago

I'm a little concerned over removing visorchannel_debug(), only because it would have been nice to have it used via a debugfs entry, just like I recently did for the visorhba sysfs entry (commit bcde4876caf356f15a9bebaaf24b0c50cb487102)

/visorhba/vbus<x>:dev<y>/info

(info_debugfs_show() in visorhba_main.c) E.g., we could have this function invoked for:

/visorbus/vbus<x>:dev<y>/visorchannel

But we can always add the code back if we decide to do this.

davidker commented 8 years ago

I've pulled this series out of upstream-next.

mriswyth commented 8 years ago

Comments addressed in updated commits. Commits pushed as githubissue#59-upstream-next_v2 branch

selltc commented 8 years ago

8/15 - Greg applied these patches to staging-next in: