[Bug 2262135] New: Review Request: Qualcomm Remote Filesystem Service Implementation

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: [e-mail]
          Reporter: [e-mail]
        QA Contact: [e-mail]
                CC: [e-mail]
  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

[Bug 2262135] Review Request: Qualcomm Remote Filesystem Service Implementation

https://bugzilla.redhat.com/show_bug.cgi?id=2262135

Javier Martinez Canillas  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

[Bug 2262135] Review Request: Qualcomm Remote Filesystem Service Implementation

https://bugzilla.redhat.com/show_bug.cgi?id=2262135

Yanko Kaneti  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #1 from Yanko Kaneti  ---
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...

[Bug 2262135] Review Request: Qualcomm Remote Filesystem Service Implementation

https://bugzilla.redhat.com/show_bug.cgi?id=2262135

--- Comment #2 from Javier Martinez Canillas  ---
(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.

[Bug 2262135] Review Request: Qualcomm Remote Filesystem Service Implementation

https://bugzilla.redhat.com/show_bug.cgi?id=2262135

Nicolas Chauvet (kwizart)  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #3 from Nicolas Chauvet (kwizart)  ---
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 ?

[Bug 2262135] Review Request: Qualcomm Remote Filesystem Service Implementation

https://bugzilla.redhat.com/show_bug.cgi?id=2262135

--- Comment #4 from Javier Martinez Canillas  ---
(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.

[Bug 2262135] Review Request: Qualcomm Remote Filesystem Service Implementation

https://bugzilla.redhat.com/show_bug.cgi?id=2262135

Yanko Kaneti  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|[e-mail]    |[e-mail]
             Status|NEW                         |ASSIGNED
              Flags|                            |fedora-review+

--- Comment #5 from Yanko Kaneti  ---
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

[Bug 2262135] Review Request: Qualcomm Remote Filesystem Service Implementation

https://bugzilla.redhat.com/show_bug.cgi?id=2262135

--- Comment #6 from Nicolas Chauvet (kwizart)  ---
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...

[Bug 2262135] Review Request: Qualcomm Remote Filesystem Service Implementation

https://bugzilla.redhat.com/show_bug.cgi?id=2262135

--- Comment #7 from Javier Martinez Canillas  ---
(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.

[Bug 2262135] Review Request: Qualcomm Remote Filesystem Service Implementation

https://bugzilla.redhat.com/show_bug.cgi?id=2262135

--- Comment #8 from Javier Martinez Canillas  ---
(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!

[Bug 2262135] Review Request: rmtfs - Qualcomm Remote Filesystem Service Implementation

https://bugzilla.redhat.com/show_bug.cgi?id=2262135

Javier Martinez Canillas  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Review Request: Qualcomm    |Review Request: rmtfs -
                   |Remote Filesystem Service   |Qualcomm Remote Filesystem
                   |Implementation              |Service Implementation

[Bug 2262135] Review Request: rmtfs - Qualcomm Remote Filesystem Service Implementation

https://bugzilla.redhat.com/show_bug.cgi?id=2262135

Fedora Admin user for bugzilla script actions  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |POST

--- Comment #9 from Fedora Admin user for bugzilla script actions  ---
The Pagure repository was created at https://src.fedoraproject.org/rpms/rmtfs

[Bug 2262135] Review Request: rmtfs - Qualcomm Remote Filesystem Service Implementation

https://bugzilla.redhat.com/show_bug.cgi?id=2262135

--- Comment #10 from Nicolas Chauvet (kwizart)  ---
> 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.

[Bug 2262135] Review Request: rmtfs - Qualcomm Remote Filesystem Service Implementation

https://bugzilla.redhat.com/show_bug.cgi?id=2262135

Yanko Kaneti  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|POST                        |CLOSED
         Resolution|---                         |CURRENTRELEASE
        Last Closed|                            |2024-11-08 10:09:55