https://bugzilla.redhat.com/show_bug.cgi?id=2262135
Bug ID: 2262135 Summary: Review Request: Qualcomm Remote Filesystem Service Implementation Product: Fedora Version: rawhide Hardware: aarch64 OS: Linux Status: NEW Component: Package Review Severity: medium Assignee: nobody@fedoraproject.org Reporter: fmartine@redhat.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://javierm.fedorapeople.org/rpms/review/rmtfs/rmtfs.spec SRPM URL: https://javierm.fedorapeople.org/rpms/review/rmtfs/rmtfs-1.0-1.fc40.src.rpm
Description: Qualcomm Remote Filesystem Service Implementation.
Fedora Account System Username: javierm
Reproducible: Always
https://bugzilla.redhat.com/show_bug.cgi?id=2262135
Javier Martinez Canillas fmartine@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Comment|0 |updated
--- Comment #0 has been edited ---
Spec URL: https://javierm.fedorapeople.org/rpms/review/rmtfs/rmtfs.spec SRPM URL: https://javierm.fedorapeople.org/rpms/review/rmtfs/rmtfs-1.0-1.fc40.src.rpm
Description: Qualcomm Remote Filesystem Service Implementation.
Fedora Account System Username: javierm
https://bugzilla.redhat.com/show_bug.cgi?id=2262135
Yanko Kaneti yaneti@declera.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |yaneti@declera.com
--- Comment #1 from Yanko Kaneti yaneti@declera.com --- This looks ok, with perhaps the exception of the missing systemd handling of rmtfs.service
https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_syste...
https://bugzilla.redhat.com/show_bug.cgi?id=2262135
--- Comment #2 from Javier Martinez Canillas fmartine@redhat.com --- (In reply to Yanko Kaneti from comment #1)
This looks ok, with perhaps the exception of the missing systemd handling of rmtfs.service
https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/ #_systemd
Thanks a lot for your feedback. I've added these now.
The Spec and SRPM files are in the same URL that was mentioned in Comment #0.
https://bugzilla.redhat.com/show_bug.cgi?id=2262135
Nicolas Chauvet (kwizart) kwizart@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |kwizart@gmail.com
--- Comment #3 from Nicolas Chauvet (kwizart) kwizart@gmail.com --- I don't see the service checking if it's running on any relevant device ? Can you make the appropriate condition so that the service only enable itself on the appropriate device ?
Also despite the package built on others arches, I see little point to have it on something else than aarch64. or is there any ?
https://bugzilla.redhat.com/show_bug.cgi?id=2262135
--- Comment #4 from Javier Martinez Canillas fmartine@redhat.com --- (In reply to Nicolas Chauvet (kwizart) from comment #3)
I don't see the service checking if it's running on any relevant device ? Can you make the appropriate condition so that the service only enable itself on the appropriate device ?
Can you give me an example of a service that does this check ? I've never seen it.
Also despite the package built on others arches, I see little point to have it on something else than aarch64. or is there any ?
The guidelines say that packagers should "Fedora packagers should make every effort to support all primary architectures":
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_architecture_sup...
Yes, arch specific code like this may be restricted to aarch64, but as packager I still find it useful to try local builds on my laptop, etc.
I don't have a strong opinion though.
https://bugzilla.redhat.com/show_bug.cgi?id=2262135
Yanko Kaneti yaneti@declera.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|nobody@fedoraproject.org |yaneti@declera.com Status|NEW |ASSIGNED Flags| |fedora-review+
--- Comment #5 from Yanko Kaneti yaneti@declera.com --- I guess stuff like this could still be useful in some testing/emulation capacity on something pretending to be a quallcom device? In any case its small and I don't think it hurts a lot having it at least build on all..this can be changed at any point.
License is OK Name is OK. Builds in koji.
APPROVED
https://bugzilla.redhat.com/show_bug.cgi?id=2262135
--- Comment #6 from Nicolas Chauvet (kwizart) kwizart@gmail.com --- if debugging or emulating, it still good to have a local build, but the packager guideline also stipulate to have a package generally useful Having an aarch64 only build would prevent the package to end in any non-aarch64 produced Fedora deliverable media.
For the unit file condition, you can have a look at mcelog on x86_64: systemctl cat mcelog
Namely: ConditionPathExists=!/sys/module/edac_mce_amd/initstate
Of course, you need to find a path that can discriminate your particular device. This is particularly useful to avoid any error on non-qcom aarch64 devices (or even qcom that do not need this driver).
An even better alternative is to use service activation with udev. (we use that for the nvidia driver at rpmfusion), like cat /usr/lib/udev/rules.d/10-nvidia.rules SUBSYSTEM=="pci", ATTRS{vendor}=="0x10de", ATTRS{class}=="0x030[02]00", TAG+="systemd", ENV{SYSTEMD_WANTS}="nvidia-fallback.service"
To be adapted as appropriate...
https://bugzilla.redhat.com/show_bug.cgi?id=2262135
--- Comment #7 from Javier Martinez Canillas fmartine@redhat.com --- (In reply to Nicolas Chauvet (kwizart) from comment #6)
if debugging or emulating, it still good to have a local build, but the packager guideline also stipulate to have a package generally useful Having an aarch64 only build would prevent the package to end in any non-aarch64 produced Fedora deliverable media.
For the unit file condition, you can have a look at mcelog on x86_64: systemctl cat mcelog
Namely: ConditionPathExists=!/sys/module/edac_mce_amd/initstate
Of course, you need to find a path that can discriminate your particular device.
This is for most Windows-on-ARM QC based laptops and QC based Chromebooks. It would be hard to maintain the allow list of devices using a combination of DMI information and Device Tree compatible strings (I can't think of any other way to discriminate).
Not worth it in my opinion.
This is particularly useful to avoid any error on non-qcom aarch64 devices (or even qcom that do not need this driver).
An even better alternative is to use service activation with udev. (we use that for the nvidia driver at rpmfusion), like cat /usr/lib/udev/rules.d/10-nvidia.rules SUBSYSTEM=="pci", ATTRS{vendor}=="0x10de", ATTRS{class}=="0x030[02]00", TAG+="systemd", ENV{SYSTEMD_WANTS}="nvidia-fallback.service"
This is a good idea and can be done as a follow-up. For now the service files are not enabled by default anyways and have to be manually enabled / started.
https://bugzilla.redhat.com/show_bug.cgi?id=2262135
--- Comment #8 from Javier Martinez Canillas fmartine@redhat.com --- (In reply to Yanko Kaneti from comment #5)
I guess stuff like this could still be useful in some testing/emulation capacity on something pretending to be a quallcom device? In any case its small and I don't think it hurts a lot having it at least build on all..this can be changed at any point.
License is OK Name is OK. Builds in koji.
APPROVED
Thanks!
https://bugzilla.redhat.com/show_bug.cgi?id=2262135
Javier Martinez Canillas fmartine@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Summary|Review Request: Qualcomm |Review Request: rmtfs - |Remote Filesystem Service |Qualcomm Remote Filesystem |Implementation |Service Implementation
https://bugzilla.redhat.com/show_bug.cgi?id=2262135
Fedora Admin user for bugzilla script actions fedora-admin-xmlrpc@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |POST
--- Comment #9 from Fedora Admin user for bugzilla script actions fedora-admin-xmlrpc@fedoraproject.org --- The Pagure repository was created at https://src.fedoraproject.org/rpms/rmtfs
https://bugzilla.redhat.com/show_bug.cgi?id=2262135
--- Comment #10 from Nicolas Chauvet (kwizart) kwizart@gmail.com ---
This is a good idea and can be done as a follow-up. For now the service files are not enabled by default anyways and have to be manually enabled / started.
Right, but enabling it by default is the action right after. Then, get it into the default media. So I don't get why it's not discussed as part of the review. This will save back and forth implementation tryouts.
My idea since it's tight to a wireless module is to activate the service only if the qcom wifi module is loaded. That way, blacklisting the wifi module (for any reason) will also prevent this service to load the firmware.
https://bugzilla.redhat.com/show_bug.cgi?id=2262135
Yanko Kaneti yaneti@declera.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |CLOSED Resolution|--- |CURRENTRELEASE Last Closed| |2024-11-08 10:09:55
package-review@lists.fedoraproject.org