linux-kernel - Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1211562837.28967.51.camel@pmac.infradead.org>
Date:	Fri, 23 May 2008 18:13:57 +0100
From:	David Woodhouse <dwmw2@...radead.org>
To:	Sam Ravnborg <sam@...nborg.org>
Cc:	linux-kernel@...r.kernel.org, aoliva@...hat.com,
	alan@...rguk.ukuu.org.uk, Abhay Salunke <Abhay_Salunke@...l.com>,
	kay.sievers@...y.org, Haroldo Gamal <gamal@...ernex.com.br>,
	Takashi Iwai <tiwai@...e.de>
Subject: Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

On Fri, 2008-05-23 at 18:41 +0200, Sam Ravnborg wrote:
> > +FIRMWARE_BINS := $(subst ",,$(CONFIG_BUILTIN_FIRMWARE))
> > +FIRMWARE_OBJS := $(patsubst %,%.o, $(FIRMWARE_BINS))
> > +FIRMWARE_SRCS := $(patsubst %,$(obj)/%.c, $(FIRMWARE_BINS))
> 
> My personal rule-of-thumb is to use lower case for
> all local variables and UPPER case for global variables.
> 
> I know that outside the kernel everyone use UPPER case
> for Makefile variables but that just not readable.
> So in this code snippet I would have used lower case.

OK, I'll do that.

> > +
> > +
> > +quiet_cmd_fwbin = MK_FW   $@
> > +      cmd_fwbin = echo '/* File automatically generated */' > $@ ;	 \
> > +		  echo '\#include <linux/firmware.h>' >> $@ ;		 \
> > +		  echo 'static const unsigned char fw[] = {' >> $@ ;	 \
> > +		  od -t x1 -A none -v $(srctree)/$(patsubst %.c,%,$@) | \
> > +		   sed -e 's/ /, 0x/g' -e 's/^,//' -e 's/$$/,/' >> $@ ; \
> > +		  echo '};' >> $@ ;					 \
> > +		  echo 'DECLARE_BUILTIN_FIRMWARE("$(patsubst firmware/%.c,%,$@)",fw);' >> $@

> A small comment is justified here.
> Do not consider everyone to know od.

Isn't the context enough? Some comments are just superfluous :)

> If you choose a deciaml output you do not need to add 0x

Hm, that's true -- force of habit, I suppose. But it doesn't really
matter, and if anyone _does_ have cause to look at the generated file,
it's probably nicer in hex.

Actually, it would be nicer to do it in assembly and use .incbin -- but
then we'd have to deal with potential struct alignment issues for the
'struct builtin_fw'. Again, that's an improvement for later. But it's
why I used 'unsigned long' for the size, instead of 'size_t'.

> > +
> > +$(FIRMWARE_SRCS): $(obj)/%.c: $(srctree)/$(obj)/%
> > +	$(call cmd,fwbin)
> > +
> > +obj-y := $(FIRMWARE_OBJS)
> 
> Assignment to targets i smiisng so generated files are not cleaned
> by "make clean".
> 
> targets := $(FIRMWARE_OBJS)

OK, thanks.

-- 
dwmw2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/