whatawurst / android_kernel_sony_msm8998

LineageOS Kernel Tree for Sony Xperia XZ Premium, XZ1 and XZ1 Compact
Other
38 stars 72 forks source link

Remove redundant `CONFIG_USB_F_*` entries from defconfigs #68

Closed Flamefire closed 1 year ago

Flamefire commented 2 years ago

Those are set by CONFIG_USB_CONFIGFS_* as far as I understand the system.

Maybe someones else (@bananafunction ? @cyberknight777 ?) Can comment on this?
See https://github.com/whatawurst/android_device_sony_yoshino-common/issues/50

From my tests the C files still get compiled, so this should be correct.

cyberknight777 commented 2 years ago

Those are set by CONFIG_USB_CONFIGFS_* as far as I understand the system.

Maybe someones else (@bananafunction ? @cyberknight777 ?) Can comment on this? See whatawurst/android_device_sony_yoshino-common#50

From my tests the C files still get compiled, so this should be correct.

Do have a read up at https://github.com/whatawurst/android_device_sony_yoshino-common/issues/50#issuecomment-1169060076 for my explanation on the subject.

Flamefire commented 2 years ago

This?

CONFIG_USBF exists for USB functions, some code in driver/usb/gadget/function/ is guarded with CONFIG_USBF

while some code in drivers/usb/gadget/configfs.c with CONFIG_USBCONFIGFS*

To me it looks like CONFIG_USB_CONFIGFS_* set implies CONFIG_USB_F_*. Only uses of the latter I see are in the Makefile and are of the form obj-$(CONFIG_USB_F_ACM) += usb_f_acm.o which I read as "Compile (only) if CONFIG_USB_F_ACM is set to y". And in 2 headers: #if IS_ENABLED(CONFIG_USB_F_QDSS) and #if IS_ENABLED(CONFIG_USB_F_DIAG)

When building this PR I noticed that e.g f_mass_storage.c still gets compiled which I take as confirmation that at least for the Makefile the above assumption is correct.

Also judging by the fact that only CONFIG_USB_CONFIGFS_UEVENT and CONFIG_USB_CONFIGFS_F_ACC are ever checked (in all current source files, namely in configfs.c and f_midi.c I have support for the 2nd part of the assumption.

I'll check and verify that CONFIG_USB_F_DIAG does indeed get defined during compilation when CONFIG_USB_CONFIGFS_F_DIAG=y is set in the defconfig but I'd assume so. Especially as only we set the CONFIG_USB_F_* in the defconfig but the default/other defconfig don't. And the commit message of https://github.com/whatawurst/android_kernel_sony_msm8998/commit/7c9990241e1309b5cdf28c5e6ee48c11524a3242 reads "Disable CONFIG_USB_F_GSI" and removes (only) CONFIG_USB_CONFIGFS_F_GSI=y

And code-wise we have: https://github.com/whatawurst/android_kernel_sony_msm8998/blob/fcc25989d748fcbe0dd13dbd05071db0038c4b93/drivers/usb/gadget/Kconfig#L274 https://github.com/whatawurst/android_kernel_sony_msm8998/blob/fcc25989d748fcbe0dd13dbd05071db0038c4b93/drivers/usb/gadget/Kconfig#L279

So it all checks but I haven't found any docu related to that.

cyberknight777 commented 2 years ago

This?

CONFIG_USBF exists for USB functions, some code in driver/usb/gadget/function/ is guarded with CONFIG_USBF while some code in drivers/usb/gadget/configfs.c with CONFIG_USBCONFIGFS*

To me it looks like CONFIG_USB_CONFIGFS_* set implies CONFIG_USB_F_*. Only uses of the latter I see are in the Makefile and are of the form obj-$(CONFIG_USB_F_ACM) += usb_f_acm.o which I read as "Compile (only) if CONFIG_USB_F_ACM is set to y". And in 2 headers: #if IS_ENABLED(CONFIG_USB_F_QDSS) and #if IS_ENABLED(CONFIG_USB_F_DIAG)

When building this PR I noticed that e.g f_mass_storage.c still gets compiled which I take as confirmation that at least for the Makefile the above assumption is correct.

Also judging by the fact that only CONFIG_USB_CONFIGFS_UEVENT and CONFIG_USB_CONFIGFS_F_ACC are ever checked (in all current source files, namely in configfs.c and f_midi.c I have support for the 2nd part of the assumption.

I'll check and verify that CONFIG_USB_F_DIAG does indeed get defined during compilation when CONFIG_USB_CONFIGFS_F_DIAG=y is set in the defconfig but I'd assume so. Especially as only we set the CONFIG_USB_F_* in the defconfig but the default/other defconfig don't. And the commit message of 7c99902 reads "Disable CONFIG_USB_F_GSI" and removes (only) CONFIG_USB_CONFIGFS_F_GSI=y

And code-wise we have:

https://github.com/whatawurst/android_kernel_sony_msm8998/blob/fcc25989d748fcbe0dd13dbd05071db0038c4b93/drivers/usb/gadget/Kconfig#L274

https://github.com/whatawurst/android_kernel_sony_msm8998/blob/fcc25989d748fcbe0dd13dbd05071db0038c4b93/drivers/usb/gadget/Kconfig#L279

So it all checks but I haven't found any docu related to that.

yeah there isn't any docs regarding this which is weird ngl

Flamefire commented 2 years ago

I'll check and verify that CONFIG_USB_F_DIAG does indeed get defined during compilation when CONFIG_USB_CONFIGFS_F_DIAG=y is set in the defconfig but I'd assume so.

Ok done. With this PR CONFIG_USB_F_DIAG isn't set in any of our defconfigs anymore, only CONFIG_USB_CONFIGFS_F_DIAG. Still the #if IS_ENABLED(CONFIG_USB_F_DIAG) check succeeds.

During the build I see

make[1]: Entering directory '/out/target/product/lilac/obj/KERNEL_OBJ'
  GEN     ./Makefile
#
# configuration written to .config
#
make[1]: Leaving directory '/out/target/product/lilac/obj/KERNEL_OBJ'
...
make[1]: Entering directory '/out/target/product/lilac/obj/KERNEL_OBJ'
  GEN     ./Makefile
scripts/kconfig/conf  --savedefconfig=defconfig Kconfig

And in that folder I see a .config file which has all those options including CONFIG_USB_CONFIGFS_F_DIAG and CONFIG_USB_F_DIAG

cyberknight777 commented 2 years ago

I'll check and verify that CONFIG_USB_F_DIAG does indeed get defined during compilation when CONFIG_USB_CONFIGFS_F_DIAG=y is set in the defconfig but I'd assume so.

Ok done. With this PR CONFIG_USB_F_DIAG isn't set in any of our defconfigs anymore, only CONFIG_USB_CONFIGFS_F_DIAG. Still the #if IS_ENABLED(CONFIG_USB_F_DIAG) check succeeds.

During the build I see

make[1]: Entering directory '/out/target/product/lilac/obj/KERNEL_OBJ'
  GEN     ./Makefile
#
# configuration written to .config
#
make[1]: Leaving directory '/out/target/product/lilac/obj/KERNEL_OBJ'
...
make[1]: Entering directory '/out/target/product/lilac/obj/KERNEL_OBJ'
  GEN     ./Makefile
scripts/kconfig/conf  --savedefconfig=defconfig Kconfig

And in that folder I see a .config file which has all those options including CONFIG_USB_CONFIGFS_F_DIAG and CONFIG_USB_F_DIAG

yes it regenerates defconfig, so you dropping it would be irrelevant.

Flamefire commented 2 years ago

yes it regenerates defconfig, so you dropping it would be irrelevant.

Well it avoids having that in 2 places requiring 2 changes instead of one, see e.g. https://github.com/Neternels/android_kernel_xiaomi_mojito/commit/43142d2e383d6d9847458581d30ef0fb964e1f45

derfelot commented 2 years ago

I just had a quick glance at this conversation, but are you saying that the entries get added to the regenerated defconfig again?

In that case, I would leave them in. Our defconfigs in the kernel source are 1:1 identical to the regenerated defconfig (it's how i kept them updated between kernel subversion bumps, etc).