Bug#840515: RFS: open-ath9k-htc-firmware/1.4.0-25-gf6af791-1 ITP #751339

2016-11-06 Thread Paul Wise
On Sat, 2016-10-15 at 10:38 +0300, Paul Fertser wrote:

> Many thanks for your review, please see some answers below.

Thanks to both of you for the explanations and updated package.

I'll discuss the ROM code from two angles: Debian & Software Freedom

1) Debian

The Debian and ftp-master policy is that all the contents of main,
including all files in the upstream source tarballs, must be DFSG-free
and also build-able using only main. For open-ath9k-htc-firmware there
are some files that AFAIK don't meet this criteria and so they should
be stripped from the orig.tar.gz (not just removed at build time).

I would suggest not just stripping all of sboot/ to preserve the
ability of offline Debian users to study the remaining source code.

The usual way to repack upstream tarballs for this purpose is to put
a Files-Excluded header with associated Comment header explaining the
issue in debian/copyright and have uscan do the tarball repack.

https://wiki.debian.org/UscanEnhancements

At the same time there are some compiled files that do seem to have
source present and are probably buildable with the toolchain built
during the build process but are probably not useful to have in the
tarball, those could be stripped too to avoid confusion.

Please correct me if I have wrong information regarding these files:

This directory seems like it is solely prebuilt files and is very
unlikely to contain freely-buildable files, since the subdirectories
are named fpga and asic and the chip was produced before the free
icestorm toolchain for Lattice FPGAs was released. I don't know if all
the source is present or not but I found a string (see below) that is
present in the ROM output dir, but not anywhere else in the tree.
To avoid having to do the work to prove that the source is present and
buildable using only Debian main, I would suggest removing this dir:

./sboot/magpie_1_1/image/output/

COMMAND TO START FIRMWARE RECEIVED

These files appear to have their source in the .S files in the
./sboot/athos/src/xtos/ directory as there are similar names there.
Removing them would avoid having to explain where the source is and
checking that it is buildable with the toolchain built by the package.

./sboot/magpie_1_1/sboot/athos/src/libhandlers-board.a
./sboot/magpie_1_1/sboot/athos/src/_vectors.o
./sboot/magpie_1_1/sboot/athos/src/crt1-tiny.o
./sboot/make_opt/lib/libhandlers-board.a
./sboot/make_opt/lib/_vectors.o
./sboot/make_opt/lib/crt1-tiny.o

The PNG files could also be removed to save a small amount of space,
but it is clear that the source is present and that we have Inkscape in
Debian main so the PNG files can be rebuilt from their SVG sources, so
this isn't strictly necessary. Also, the removal could happen upstream
and a Makefile could be added using rsvg to allow building the PNGs,
if they are needed to be present for a particular reason.

2) Software Freedom

From a Software Freedom PoV, the mask ROM is bad because it is truly
read-only software that users can't modify. OTOH, it reduces RAM needed
by the firmware so it enables users to do more with the device.

This is just an idea for improving the available software freedom for
users of this firmware, not something I expect to be done any time soon:

Ideally the entire ROM source code could be reproducibly built and
during the default build it would compile the code and then compare the
resulting hash against the known ROM hash.

When someone wants to experiment with modifying a section of the code
(a function or similar), the build happens as per normal but they
request that certain functions be placed into the firmware for loading
into RAM instead.

There could also be a test mode where as much of the ROM code is added
to RAM as possible and then the RAM version is tested to see if it
behaves the same as the ROM version. 

Some further thoughts on other things:

The debian/copyright file does not appear to be fully correct, for
example a number of files in the sboot/ directory look like MIT, X11 or
 Expat licenses rather than BSD-3-clause-from-Qualcomm-Atheros.
This would need to be fixed before uploading to Debian.
I noticed this in the output of `debmake -k` btw.

It might be interesting to have isenkram/apt/etc suggest this firmware
when someone plugs in a device that can run this firmware.

https://wiki.debian.org/AppStream/Guidelines#Announcing_supported_hardware

> This one makes me wonder if it'd be the correct way to go. I assume
> only the copyright holder can do this, and spending time contacting
> eCos developers (and probably lawyers) just to get it right for some
> really old code seems impractical. I'd consider this to be an upstream
> issue unrelated to Debian packaging. Please correct me if I'm wrong.

Yes, this is an upstream issue. I don't think changing the address
needs contacting the copyright holders since it is mainly informational
rather than part of the license grant (IMO). It is part of a comment
so presumably changing it could have no 

Bug#840515: RFS: open-ath9k-htc-firmware/1.4.0-25-gf6af791-1 ITP #751339

2016-10-15 Thread Paul Fertser
Hey Paul,

Many thanks for your review, please see some answers below.

On Thu, Oct 13, 2016 at 11:55:29AM +0800, Paul Wise wrote:
> > Cc: Oleksij Rempel 
> 
> Please use the X-Debbugs-CC pseudo-header when submitting bugs instead

Noted. Nice feature.

> > I am looking for a sponsor for my package "open-ath9k-htc-firmware"
> 
> Correct me if I'm wrong but IIRC either yourself or Oleksij have
> commit/release access upstream?

Well, basically Oleksij co-maintains the upstream repository with
Adrian Chadd (former QCA employee).

I have commit rights for the packaging branch in Oleksij's personal
repo.

> The comments for the overrides for lintian tag source-is-missing
> should indicate which file is the source for each prebuilt file. I
> would have one comment per instance of the tag.

In this particular case it's unclear if the source is really missing
or not. If you say the upstream sources absolutely MUST be repackaged
if they include binaries that we can't get the sources for even if the
said binaries are removed automatically before building and are not
needed for the process anyhow, well, I'll try to investigate the
details and will come back with the answer.

> Just one comment saying they are removed at build time is enough for
> all of the overrides for lintian tag
> source-contains-prebuilt-binary.

OK.

> Personally, I would suggest removing all of these files from the
> upstream git repository and always building them from source. If you
> need to keep them, put them in tarballs in the github releases.

Not practical, please see below.

> Personally, I would recommend the generated files docs/*.png be
> removed from the upstream git repo and rendered at build time from
> their source SVG files if they are needed.

Noted. Probably not anything to worry about if we'll have to repack
upstream sources anyway due to proprietary objects, the *.png will be
removed along the way.

> I think you should also remove the prebuilt files before
> dh_auto_configure so that they can never get used by a build, even a
> manual one where `debian/rules clean` is not run.
...
> I would use a debian/clean file instead of override_dh_clean.

Noted.

> What is the reason for removing the docs/ and sboot/ directories in
> override_dh_clean? AFAICS both of these contain source files. IMO you
> should just remove the generated files.

Since neither are needed for building the actual firmware, it's easier
to remove them completely rather than mess with individual files.

The device hardware includes not only the microcontroller but also
some RAM and ROM. The tricky part here is that ROM is a mask ROM one,
that is, truly read-only. During the development a set of generic
useful functions was identified and QCA produced a batch of ICs with
those functions present in the mask ROM memory. The main firmware is
stored on a host device and is loaded dynamically to target's RAM. To
cut down on the amount of RAM needed, it makes use of those generic
functions stored in ROM. sboot/ directory contains (probably partial,
will have to investigate this further, if absolutely needed; probably
repacking the upstream tarball would be easier though) sources for the
said ROM as well as the binaries that are supposed to be identical to
the ROM's contents. During the build some header files and linker
scripts are used to provide external linkage to the generic functions,
the said headers and scripts are placed in the target_firmware/
directory, so sboot/ can be removed completely. For some debugging
purposes it might be beneficial to inspect the sources of the
mentioned functions and sometimes even the resulting binaries. So
sboot/ should probably stay in the upstream repository for the
reference purposes. Making upstream package it separately sounds like
additional hassle for no practical gain (except for the Debian
packaging purposes probably), it's way easier to have a single git
repository contain everything useful for development and debugging.

> The cmake part of the build process does not print compiler
> invocation. Debian requires full compiler flags/output in build logs.
> For cmake usually debhelper automatically passes
> -DCMAKE_VERBOSE_MAKEFILE=ON but it doesn't seem to be working here?

Will investigate.

> The debian/watch file needs adjusting for the new source package name:
> s/firmware-ath9k-htc/open-ath9k-htc-firmware/
> 
> Personally I would leave "debian uupdate" out of the watch file
> because it can be annoying for people who just want to download.

Indeed. I'm new to this, so just copied the example from the uscan
manpage.

> Please get the FSF address corrected in the upstream copyright info.

This one makes me wonder if it'd be the correct way to go. I assume
only the copyright holder can do this, and spending time contacting
eCos developers (and probably lawyers) just to get it right for some
really old code seems impractical. I'd consider this to be an upstream
issue unrelated to Debian 

Bug#840515: RFS: open-ath9k-htc-firmware/1.4.0-25-gf6af791-1 ITP #751339

2016-10-13 Thread Oleksij Rempel
Am 13.10.2016 um 05:55 schrieb Paul Wise:
> On Wed, Oct 12, 2016 at 7:38 PM, Paul Fertser wrote:
> 
>> Cc: Oleksij Rempel 
> 
> Please use the X-Debbugs-CC pseudo-header when submitting bugs instead
> of CC, so that the recipients get the bug number too. Put it just
> after Severity in the mail body so those CCed can see it:
> 
> https://www.debian.org/Bugs/Reporting#xcc
> 
>> I am looking for a sponsor for my package "open-ath9k-htc-firmware"
> 
> Correct me if I'm wrong but IIRC either yourself or Oleksij have
> commit/release access upstream?

correct.

> The comments for the overrides for lintian tag source-is-missing
> should indicate which file is the source for each prebuilt file. I
> would have one comment per instance of the tag. Just one comment
> saying they are removed at build time is enough for all of the
> overrides for lintian tag source-contains-prebuilt-binary. Personally,
> I would suggest removing all of these files from the upstream git
> repository and always building them from source. If you need to keep
> them, put them in tarballs in the github releases.

I can answer this part, especially because most of comments are related
to sboot/ folder. sboot contains ROM code flashed to the chip or eeprom.
Not all ROM parts seems to be open (fix me if i'm wrong), but at least
contain some binary. If some person will decide to RE closed parts, it
will be easier to know what exactly should be RE instead  of work on
complete ROM.
No one will ever touch/patch  sboot. At least i will not allow it.
The actual firmware is located in RAM and to reduce memory usage, it is
using ROM functions. To understand what is used and to be able to fix
any thing in the firmware you need to read sboot. The sboot should
reflect state of the code with all this bugs and problems. Even if this
is wrong FSF address.

For most paranoid persons we remove sboot before build is started.

> Personally, I would recommend the generated files docs/*.png be
> removed from the upstream git repo and rendered at build time from
> their source SVG files if they are needed.

ok, i'll remove pngs.

> I think you should also remove the prebuilt files before
> dh_auto_configure so that they can never get used by a build, even a
> manual one where `debian/rules clean` is not run.
> 
> What is the reason for removing the docs/ and sboot/ directories in
> override_dh_clean? AFAICS both of these contain source files. IMO you
> should just remove the generated files.
> 
> The cmake part of the build process does not print compiler
> invocation. Debian requires full compiler flags/output in build logs.
> For cmake usually debhelper automatically passes
> -DCMAKE_VERBOSE_MAKEFILE=ON but it doesn't seem to be working here?
> 
> The debian/watch file needs adjusting for the new source package name:
> s/firmware-ath9k-htc/open-ath9k-htc-firmware/
> 
> Personally I would leave "debian uupdate" out of the watch file
> because it can be annoying for people who just want to download.
> 
> I would use a debian/clean file instead of override_dh_clean.
> 
> Please get the FSF address corrected in the upstream copyright info.
> 
> debian/cross-toolchain.mk needs copyright/license info filled in,
> preferably in both the header of it and in debian/control.
> 
> The Uploaders field is empty. I would have expected to your name there
> if you intend to co-maintain it with Oleksij.
> 
> I would recommend running this command (from the devscripts package)
> so that future diffs of the Debian packaging are easier to read:
> 
> wrap-and-sort --short-indent --wrap-always --sort-binary-packages
> --trailing-comma
> 
> The Vcs-* fields should point at the repository containing the Debian
> packaging, not upstream:
> 
> https://www.debian.org/doc/debian-policy/ch-controlfields.html#s-f-VCS-fields
> 
> The Conflicts/Suggests fields are empty, you can remove them from
> debian/control.
> 
> For merged bugs, you only need to close one of them and the rest will
> be closed too.
> 
> What is the reason for renaming the upstream firmware files? Does the
> Linux kernel detect the new names?

upstream file names have old name schema without version numbers. Since
we are not able to guarantee 100% backwards compatibility and testing
coverage for all available kernel version we need to have different fw
binaries for different version. For example linux-firmware repo contains
both 1.3 and 1.4 binaries.
The same is about debian firmware-atheros packet, it contains:
/lib/firmware/ath9k_htc/htc_7010-1.4.0.fw
/lib/firmware/ath9k_htc/htc_9271-1.4.0.fw
/lib/firmware/htc_7010.fw
/lib/firmware/htc_9271.fw

Only way to avoid conflict between packages and be able to provide FW
version different from actual realise, for example with security fixes
is to use /lib/firmware/ath9k_htc/htc_9271-1.dev.0.fw name. To tell the
kernel that this should be used, we need to provide ath9k_htc.conf

At same time Adrian Chadd is promising to provide BSD driver for this
HW. 

Bug#840515: RFS: open-ath9k-htc-firmware/1.4.0-25-gf6af791-1 ITP #751339

2016-10-12 Thread Paul Wise
On Wed, Oct 12, 2016 at 7:38 PM, Paul Fertser wrote:

> Cc: Oleksij Rempel 

Please use the X-Debbugs-CC pseudo-header when submitting bugs instead
of CC, so that the recipients get the bug number too. Put it just
after Severity in the mail body so those CCed can see it:

https://www.debian.org/Bugs/Reporting#xcc

> I am looking for a sponsor for my package "open-ath9k-htc-firmware"

Correct me if I'm wrong but IIRC either yourself or Oleksij have
commit/release access upstream?

The comments for the overrides for lintian tag source-is-missing
should indicate which file is the source for each prebuilt file. I
would have one comment per instance of the tag. Just one comment
saying they are removed at build time is enough for all of the
overrides for lintian tag source-contains-prebuilt-binary. Personally,
I would suggest removing all of these files from the upstream git
repository and always building them from source. If you need to keep
them, put them in tarballs in the github releases.

Personally, I would recommend the generated files docs/*.png be
removed from the upstream git repo and rendered at build time from
their source SVG files if they are needed.

I think you should also remove the prebuilt files before
dh_auto_configure so that they can never get used by a build, even a
manual one where `debian/rules clean` is not run.

What is the reason for removing the docs/ and sboot/ directories in
override_dh_clean? AFAICS both of these contain source files. IMO you
should just remove the generated files.

The cmake part of the build process does not print compiler
invocation. Debian requires full compiler flags/output in build logs.
For cmake usually debhelper automatically passes
-DCMAKE_VERBOSE_MAKEFILE=ON but it doesn't seem to be working here?

The debian/watch file needs adjusting for the new source package name:
s/firmware-ath9k-htc/open-ath9k-htc-firmware/

Personally I would leave "debian uupdate" out of the watch file
because it can be annoying for people who just want to download.

I would use a debian/clean file instead of override_dh_clean.

Please get the FSF address corrected in the upstream copyright info.

debian/cross-toolchain.mk needs copyright/license info filled in,
preferably in both the header of it and in debian/control.

The Uploaders field is empty. I would have expected to your name there
if you intend to co-maintain it with Oleksij.

I would recommend running this command (from the devscripts package)
so that future diffs of the Debian packaging are easier to read:

wrap-and-sort --short-indent --wrap-always --sort-binary-packages
--trailing-comma

The Vcs-* fields should point at the repository containing the Debian
packaging, not upstream:

https://www.debian.org/doc/debian-policy/ch-controlfields.html#s-f-VCS-fields

The Conflicts/Suggests fields are empty, you can remove them from
debian/control.

For merged bugs, you only need to close one of them and the rest will
be closed too.

What is the reason for renaming the upstream firmware files? Does the
Linux kernel detect the new names?

What is the reason for debian/ath9k_htc.conf? Why is it
Debian-specific instead of being committed upstream?

I'd recommend either passing --parallel at the end of the args to dh,
or upgrading to debian/compat 10, which uses that by default.

Please add some upstream metadata:

https://wiki.debian.org/UpstreamMetadata

I'd suggest signing all tarballs, tags and commits with OpenPGP and
having uscan verify the tarball sigs:

https://wiki.debian.org/Creating%20signed%20GitHub%20releases
https://mikegerwitz.com/papers/git-horror-story
https://wiki.debian.org/debian/watch#Cryptographic_signature_verification
https://help.riseup.net/en/security/message-security/openpgp/best-practices

Automatic checks:

build:

Various gcc and other warnings.

lintian:

P: open-ath9k-htc-firmware source: debian-watch-may-check-gpg-signature
P: firmware-ath9k-htc: no-upstream-changelog
I: firmware-ath9k-htc: extended-description-is-probably-too-short
P: firmware-ath9k-htc-dbgsym: no-upstream-changelog
I: firmware-ath9k-htc-dbgsym: extended-description-is-probably-too-short

check-all-the-things:

$ env PERL5OPT=-m-lib=. cme check dpkg
Warning in 'copyright Format' value
'http://www.debian.org/doc/packaging-manuals/copyright-format/1.0/':
Format uses insecure http protocol instead of https
you can try 'cme fix dpkg' to fix the warnings shown above

$ codespell --quiet-level=3


$ cppcheck -j1 --quiet -f .
[sboot/magpie_1_1/sboot/athos/src/xtos/xtos-internal.h:142]: (error)
syntax error
[sboot/utility/adjust_dep/adj_dep.c:63]: (error) Buffer overrun
possible for long command line arguments.
[sboot/utility/adjust_time/adj_time.c:57]: (error) Buffer overrun
possible for long command line arguments.
[sboot/utility/adjust_time/adj_time.c:58]: (error) Buffer overrun
possible for long command line arguments.
[sboot/utility/bin2hex/bin2hex.c:197]: (error) Buffer overrun possible
for long command 

Bug#840515: RFS: open-ath9k-htc-firmware/1.4.0-25-gf6af791-1 ITP #751339

2016-10-12 Thread Paul Fertser
Package: sponsorship-requests
Severity: wishlist

Dear mentors,

I am looking for a sponsor for my package "open-ath9k-htc-firmware"

* Package name: open-ath9k-htc-firmware
  Version : 1.4.0-25-gf6af791-1
  Upstream Author : QCA
* URL : https://github.com/qca/open-ath9k-htc-firmware
* License : BSD-3-clause
  Section : misc

It builds those binary packages:

  firmware-ath9k-htc - QCA ath9k-htc Firmware
  firmware-ath9k-htc-dbgsym - QCA ath9k-htc Firmware ELF file

To access further information about this package, please visit the following 
URL:

https://mentors.debian.net/package/open-ath9k-htc-firmware


Alternatively, one can download the package with dget using this command:

  dget -x 
https://mentors.debian.net/debian/pool/main/o/open-ath9k-htc-firmware/open-ath9k-htc-firmware_1.4.0-25-gf6af791-1.dsc

Regards,
  Oleksij Rempel
  Paul Fertser