Em Fri, 13 Oct 2017 14:46:35 +0900
<[email protected]> escreveu:
> From: Yasunari Takiguchi <[email protected]>
>
> Hi,
>
> This is the patch series (version 4) of Sony CXD2880 DVB-T2/T tuner +
> demodulator driver.The driver supports DVB-API and interfaces through
> SPI.
>
> We have tested the driver on Raspberry Pi 3 and got picture and sound
> from a media player.
>
> The change history of this patch series is as below.
Finally had time to review this patch series. Sorry for taking so long.
4Q is usually very busy.
I answered each patch with comments. There ones I didn't comment
(patches 1, 4 and 8-12) is because I didn't see anything wrong,
except for the notes I did on other patches, mainly:
- they lack SPDX license text;
- the error codes need to review.
Additionally, on patches 8-11, I found weird to not found any
64 bits division, but I noticed some code there that it seemed
to actually be doing such division using some algorithm to make
them work on 32 bits machine. If so, please replace them by
do_div64(), as it makes clearer about what's been doing, and it
provides better performance when compiled on 64 bit Kernels (as
they become just a DIV asm operation).
Regards,
Mauro
Thanks,
Mauro
Dear Mauro
Thanks for your review.
We will refer to your comments and consider how to respond for them.
I want to confirm one thing about SPDX license text
We will add SPDX license text to our files,
Is it necessary to add SPDX not only .c .h Makefile but also Kconfig?
When I checked current files in driver/media, there is no Kconfig file which has SPDX.
Best Regards,
Takiguchi
Em Thu, 14 Dec 2017 09:59:32 +0000
"Takiguchi, Yasunari" <[email protected]> escreveu:
> Dear Mauro
>
> Thanks for your review.
>
> We will refer to your comments and consider how to respond for them.
> I want to confirm one thing about SPDX license text
>
> We will add SPDX license text to our files,
> Is it necessary to add SPDX not only .c .h Makefile but also Kconfig?
> When I checked current files in driver/media, there is no Kconfig file which has SPDX.
SPDX is a new requirement that started late on Kernel 4.14 development
cycle (and whose initial changes were merged directly at Linus tree).
Not all existing files have it yet, as identifying the right license
on existing files is a complex task, but if you do a:
$ git grep SPDX $(find . -name Makefile) $(find . -name Kconfig)
You'll see that lot of such files have it already.
So, yes, please add it to both Makefile and Kconfig.
Thanks,
Mauro
Dear Mauro,
On Thu, Dec 14, 2017 at 11:55 AM, Mauro Carvalho Chehab
<[email protected]> wrote:
> SPDX is a new requirement that started late on Kernel 4.14 development
> cycle (and whose initial changes were merged directly at Linus tree).
> Not all existing files have it yet, as identifying the right license
> on existing files is a complex task, but if you do a:
>
> $ git grep SPDX $(find . -name Makefile) $(find . -name Kconfig)
>
> You'll see that lot of such files have it already.
FWIW, short of having SPDX tags, identifying the right license on
existing files is not a super complex task: if boils down to running
many diffs.
Take the ~60K files in kernel, and about 6K license and notices
reference texts. Then compute a pairwise diff of each of the 60K file
against the 6K reference texts. Repeat the pairwise diff a few more
times, say 10 times, as multiple licenses may appear in any given
kernel file. And keep the diffs that have the fewest
difference/highest similarity with the reference texts as the detected
license. Done!
The only complex thing is that if you have a fast diff that runs at
0.1 millisec end-to-end per diff, you still have 3.6B diffs to do and
this would take about 250 days on one thread. Even with a beefy 250
core CPU, that would still be a day (and quite few kilo watts) . So
the whole trick is to avoid doing a diffs if not really needed. This
is what I do in my scancode-toolkit (that I used/use to help Greg and
Thomas with kernel license scans). Net effect is that on a laptop on 8
threads it takes ~20 minutes to scan a whole kernel using this
diff-based approach and obtain a fairly accurate license detection.
--
Cordially
Philippe Ombredanne
> -----Original Message-----
> From: Philippe on Thursday, December 14, 2017 6:25 AM
> Dear Mauro,
>
> On Thu, Dec 14, 2017 at 11:55 AM, Mauro Carvalho Chehab
> <[email protected]> wrote:
>
> > SPDX is a new requirement that started late on Kernel 4.14 development
> > cycle (and whose initial changes were merged directly at Linus tree).
> > Not all existing files have it yet, as identifying the right license
> > on existing files is a complex task, but if you do a:
> >
> > $ git grep SPDX $(find . -name Makefile) $(find . -name Kconfig)
> >
> > You'll see that lot of such files have it already.
>
> FWIW, short of having SPDX tags, identifying the right license on
> existing files is not a super complex task: if boils down to running
> many diffs.
>
> Take the ~60K files in kernel, and about 6K license and notices
> reference texts. Then compute a pairwise diff of each of the 60K file
> against the 6K reference texts. Repeat the pairwise diff a few more
> times, say 10 times, as multiple licenses may appear in any given
> kernel file. And keep the diffs that have the fewest
> difference/highest similarity with the reference texts as the detected
> license. Done!
You can't do license detection and assignment in this automated fashion -
at least not generally.
Even a single word of difference between the notice in the source
code and the reference license notice or text may have legal implications
that are not conveyed by the simplified SPDX tag. When differences are
found, we're going to have to kick the discrepancies to a human for review.
This is especially true for files with multiple licenses.
For a work of original authorship, or a single copyright holder, the author
or copyright holder may be able to change the notice or text, or gloss
over any difference from the reference text, and make the SPDX assignment
(or even change the license, if they want). This would apply to something
new like this Sony driver. However, for code that is already in the kernel
tree, with likely multiple contributors, the legal situation gets a little
more murky.
I suspect the vast majority of the ~60k files will probably fall neatly into an
SPDX category, but I'm guessing a fair number (maybe hundreds) will require
some review and discussion.
-- Tim
Em Thu, 14 Dec 2017 17:32:34 +0000
"Bird, Timothy" <[email protected]> escreveu:
> > -----Original Message-----
> > From: Philippe on Thursday, December 14, 2017 6:25 AM
> > Dear Mauro,
> >
> > On Thu, Dec 14, 2017 at 11:55 AM, Mauro Carvalho Chehab
> > <[email protected]> wrote:
> >
> > > SPDX is a new requirement that started late on Kernel 4.14 development
> > > cycle (and whose initial changes were merged directly at Linus tree).
> > > Not all existing files have it yet, as identifying the right license
> > > on existing files is a complex task, but if you do a:
> > >
> > > $ git grep SPDX $(find . -name Makefile) $(find . -name Kconfig)
> > >
> > > You'll see that lot of such files have it already.
> >
> > FWIW, short of having SPDX tags, identifying the right license on
> > existing files is not a super complex task: if boils down to running
> > many diffs.
> >
> > Take the ~60K files in kernel, and about 6K license and notices
> > reference texts. Then compute a pairwise diff of each of the 60K file
> > against the 6K reference texts. Repeat the pairwise diff a few more
> > times, say 10 times, as multiple licenses may appear in any given
> > kernel file. And keep the diffs that have the fewest
> > difference/highest similarity with the reference texts as the detected
> > license. Done!
>
> You can't do license detection and assignment in this automated fashion -
> at least not generally.
>
> Even a single word of difference between the notice in the source
> code and the reference license notice or text may have legal implications
> that are not conveyed by the simplified SPDX tag. When differences are
> found, we're going to have to kick the discrepancies to a human for review.
> This is especially true for files with multiple licenses.
>
> For a work of original authorship, or a single copyright holder, the author
> or copyright holder may be able to change the notice or text, or gloss
> over any difference from the reference text, and make the SPDX assignment
> (or even change the license, if they want). This would apply to something
> new like this Sony driver. However, for code that is already in the kernel
> tree, with likely multiple contributors, the legal situation gets a little
> more murky.
Precisely. This is easily fixable when the code author changes the
license text, or when someone from the Company that holds copyrights sends
the SPDX markups (as emails @company can be seen as official e-mails, except
if explicitly noticed otherwise). So, from my side, I'm now requiring
SPDX for new drivers.
However, if someone else is doing the changes, it can be tricky and risky
to pick up the patch, adding my SOB, if not endorsed by the copyright
owners, or by LF legal counseling. So, I prefer to not pick those myself,
except from people I trust.
> I suspect the vast majority of the ~60k files will probably fall neatly into an
> SPDX category, but I'm guessing a fair number (maybe hundreds) will require
> some review and discussion.
Thanks,
Mauro
Tim,
On Thu, Dec 14, 2017 at 6:32 PM, Bird, Timothy <[email protected]> wrote:
>
>
>> -----Original Message-----
>> From: Philippe on Thursday, December 14, 2017 6:25 AM
>> Dear Mauro,
>>
>> On Thu, Dec 14, 2017 at 11:55 AM, Mauro Carvalho Chehab
>> <[email protected]> wrote:
>>
>> > SPDX is a new requirement that started late on Kernel 4.14 development
>> > cycle (and whose initial changes were merged directly at Linus tree).
>> > Not all existing files have it yet, as identifying the right license
>> > on existing files is a complex task, but if you do a:
>> >
>> > $ git grep SPDX $(find . -name Makefile) $(find . -name Kconfig)
>> >
>> > You'll see that lot of such files have it already.
>>
>> FWIW, short of having SPDX tags, identifying the right license on
>> existing files is not a super complex task: if boils down to running
>> many diffs.
>>
>> Take the ~60K files in kernel, and about 6K license and notices
>> reference texts. Then compute a pairwise diff of each of the 60K file
>> against the 6K reference texts. Repeat the pairwise diff a few more
>> times, say 10 times, as multiple licenses may appear in any given
>> kernel file. And keep the diffs that have the fewest
>> difference/highest similarity with the reference texts as the detected
>> license. Done!
>
> You can't do license detection and assignment in this automated fashion -
> at least not generally.
>
> Even a single word of difference between the notice in the source
> code and the reference license notice or text may have legal implications
> that are not conveyed by the simplified SPDX tag. When differences are
> found, we're going to have to kick the discrepancies to a human for review.
> This is especially true for files with multiple licenses.
>
> For a work of original authorship, or a single copyright holder, the author
> or copyright holder may be able to change the notice or text, or gloss
> over any difference from the reference text, and make the SPDX assignment
> (or even change the license, if they want). This would apply to something
> new like this Sony driver. However, for code that is already in the kernel
> tree, with likely multiple contributors, the legal situation gets a little
> more murky.
>
> I suspect the vast majority of the ~60k files will probably fall neatly into an
> SPDX category, but I'm guessing a fair number (maybe hundreds) will require
> some review and discussion.
You are completely and 100% right. I was just describing the mechanics
of the license detection side. The actual process has been and always
will be scan then review carefully and then discuss. There is no sane
automated tool that could do it all for sure.
As a funny side note, there are over 80+ licenses in the kernel and
there is (or rather was before starting adding SPDX tags) 1000+
different license notices and over 700+ variations of "this file in
under the GPL"... This starts to diminish a bit with the addition of
SPDX tags and eventually most or all boilerplate could be removed over
time with reviews and discussions, IMHO for the better: I will then be
able to trash my tool and use a good ole grep instead ;)
--
Cordially
Philippe Ombredanne
On Thu, Dec 14, 2017 at 7:04 PM, Mauro Carvalho Chehab
<[email protected]> wrote:
> Em Thu, 14 Dec 2017 17:32:34 +0000
> "Bird, Timothy" <[email protected]> escreveu:
>
>> > -----Original Message-----
>> > From: Philippe on Thursday, December 14, 2017 6:25 AM
>> > Dear Mauro,
>> >
>> > On Thu, Dec 14, 2017 at 11:55 AM, Mauro Carvalho Chehab
>> > <[email protected]> wrote:
>> >
>> > > SPDX is a new requirement that started late on Kernel 4.14 development
>> > > cycle (and whose initial changes were merged directly at Linus tree).
>> > > Not all existing files have it yet, as identifying the right license
>> > > on existing files is a complex task, but if you do a:
>> > >
>> > > $ git grep SPDX $(find . -name Makefile) $(find . -name Kconfig)
>> > >
>> > > You'll see that lot of such files have it already.
>> >
>> > FWIW, short of having SPDX tags, identifying the right license on
>> > existing files is not a super complex task: if boils down to running
>> > many diffs.
>> >
>> > Take the ~60K files in kernel, and about 6K license and notices
>> > reference texts. Then compute a pairwise diff of each of the 60K file
>> > against the 6K reference texts. Repeat the pairwise diff a few more
>> > times, say 10 times, as multiple licenses may appear in any given
>> > kernel file. And keep the diffs that have the fewest
>> > difference/highest similarity with the reference texts as the detected
>> > license. Done!
>>
>> You can't do license detection and assignment in this automated fashion -
>> at least not generally.
>>
>> Even a single word of difference between the notice in the source
>> code and the reference license notice or text may have legal implications
>> that are not conveyed by the simplified SPDX tag. When differences are
>> found, we're going to have to kick the discrepancies to a human for review.
>> This is especially true for files with multiple licenses.
>>
>> For a work of original authorship, or a single copyright holder, the author
>> or copyright holder may be able to change the notice or text, or gloss
>> over any difference from the reference text, and make the SPDX assignment
>> (or even change the license, if they want). This would apply to something
>> new like this Sony driver. However, for code that is already in the kernel
>> tree, with likely multiple contributors, the legal situation gets a little
>> more murky.
>
> Precisely. This is easily fixable when the code author changes the
> license text, or when someone from the Company that holds copyrights sends
> the SPDX markups (as emails @company can be seen as official e-mails, except
> if explicitly noticed otherwise). So, from my side, I'm now requiring
> SPDX for new drivers.
>
> However, if someone else is doing the changes, it can be tricky and risky
> to pick up the patch, adding my SOB, if not endorsed by the copyright
> owners, or by LF legal counseling. So, I prefer to not pick those myself,
> except from people I trust.
Exactly, and this why --after the first batch that Greg pushed and
Linus pulled and that had been carefully reviewed--, I am trying to
gently nit the submitters of new patches, one at a time to use the new
SPDX tags. Eventually if I can find the time, I could also submit some
bigger patches to add SPDX tags to a bunch of files at once but that
would have to be organized in small batches by copyright holder and
these would be only RFCs until reviewed by and agreed to by the actual
copyright holders.
--
Cordially
Philippe Ombredanne
Dear Mauro
I am creating patch files for version 5 of our driver.
I need to add our driver information into /drivers/media/dvb-frontends/Kconfig.
This Kconfig has no SPDX license Identifier now.
Is it unnecessary for us to add SPDX into /drivers/media/dvb-frontends/Kconfig?
(Should we add our driver information only into /drivers/media/dvb-frontends/Kconfig?)
Regards,
Takiguchi
Dear Mauro
>
> I am creating patch files for version 5 of our driver.
> I need to add our driver information into
> /drivers/media/dvb-frontends/Kconfig.
> This Kconfig has no SPDX license Identifier now.
> Is it unnecessary for us to add SPDX into
> /drivers/media/dvb-frontends/Kconfig?
> (Should we add our driver information only into
> /drivers/media/dvb-frontends/Kconfig?)
Additionally,
I need to add our driver information into
driver/media/spi/Makefile and driver/media/spi/Kconfig,
But these Makefile and Kconfig also has
no SPDX license Identifier now.
Should I keep no SPDX for them also?
Best Regards,
Takiguchi