* Re: [patch 1/3] ps3: Disk Storage Driver
From: Andrew Morton @ 2007-07-19 9:10 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Jens Axboe, James E.J. Bottomley, linux-scsi, Jens Axboe,
linux-kernel, Alessandro Rubini, linuxppc-dev, Paul Mackerras
In-Reply-To: <Pine.LNX.4.62.0707191039430.8661@pademelon.sonytel.be>
On Thu, 19 Jul 2007 10:57:53 +0200 (CEST) Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
> Hi Andrew,
>
> On Wed, 18 Jul 2007, Andrew Morton wrote:
> > On Mon, 16 Jul 2007 18:15:40 +0200
> > Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
> >
> > > From: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
> > >
> > > Add a Disk Storage Driver for the PS3:
> >
> > Your patchset significantly hits powerpc, scsi and block. So who gets to
> > merge this? Jens? James? Paul?
> >
> > Me, I guess ;)
>
> As Paul is on holidays, please take it.
OK.
> The PS3 storage driver core support is
> already in mainline, but the actual drivers aren't, as Paul was waiting for
> acks from the maintainers.
>
> BTW, do you prefer incremental patches for the comments below, or an update of
> the full patchset?
Incremental is preferred, but I convert replacement patches into
incremental patches at about 10Hz nowadays. Whatever's easier.
(Actually, if it's a replacement patch then only I get to see the
incremental patch, and the incremental patch is more reviewer-friendly).
> I didn't have much choice, as most of it was static and I don't need the full
> libata core anyway.
>
> If I would factor it out, any good suggestion where to put the factored out
> code?
Take it up with Jeff, please. If you're keen. It isn't a lot of code.
>
> > > +static int ps3disk_remove(struct ps3_system_bus_device *_dev)
> > > +{
> > > + struct ps3_storage_device *dev = to_ps3_storage_device(&_dev->core);
> > > + struct ps3disk_private *priv = ps3disk_priv(dev);
> > > +
> > > + __clear_bit(priv->gendisk->first_minor / PS3DISK_MINORS,
> > > + &ps3disk_mask);
> >
> > I see no locking here which makes this __clear_bit and the above __set_bit
> > non-racy?
>
> Were .probe()/.remove() made concurrent again? I thought that idea was dropped
> because it caused too many problems?
I don't _think_ there's any global exclusion on ->probe calls. For a
particular driver instance it's hard to see how these thigns can run
concurrently, dunno. I guess two hotunplugs coud happen concurrently.
^ permalink raw reply
* Re: [patch 2/3] ps3: BD/DVD/CD-ROM Storage Driver
From: Geert Uytterhoeven @ 2007-07-19 9:02 UTC (permalink / raw)
To: Andrew Morton
Cc: Jens Axboe, James E.J. Bottomley, linux-scsi, linux-kernel,
Alessandro Rubini, linuxppc-dev, Paul Mackerras
In-Reply-To: <20070718164351.a92ec032.akpm@linux-foundation.org>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1464 bytes --]
On Wed, 18 Jul 2007, Andrew Morton wrote:
> > +struct ps3rom_private {
> > + struct ps3_storage_device *dev;
> > + struct scsi_cmnd *curr_cmd;
> > +};
> > +#define ps3rom_priv(dev) ((dev)->sbd.core.driver_data)
> > +
>
> Someone should invent a keyboard which delivers an electric shock when the
> operator types "#define". In the meanwhile, I get to do the honours.
>
> Please don't implement in a macro anything which can be implemented in C.
All I needed was a shorthand to access driver_data, for both read and write
access (you cannot do the latter with C, unless you decouple read and write).
> > + kaddr = kmap_atomic(sgpnt->page, KM_IRQ0);
> > + if (!kaddr)
> > + return -1;
>
> kmap_atomic() cannot fail. On i386, at least. If it can fail on any other
> arch then we have a biiiiig problem. (Multiple instances of this)
Thanks, fixed!
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/
Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619
^ permalink raw reply
* Re: [patch 1/3] ps3: Disk Storage Driver
From: Geert Uytterhoeven @ 2007-07-19 8:57 UTC (permalink / raw)
To: Andrew Morton
Cc: Jens Axboe, James E.J. Bottomley, linux-scsi, Jens Axboe,
linux-kernel, Alessandro Rubini, linuxppc-dev, Paul Mackerras
In-Reply-To: <20070718163637.3f0e0164.akpm@linux-foundation.org>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 5237 bytes --]
Hi Andrew,
On Wed, 18 Jul 2007, Andrew Morton wrote:
> On Mon, 16 Jul 2007 18:15:40 +0200
> Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
>
> > From: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
> >
> > Add a Disk Storage Driver for the PS3:
>
> Your patchset significantly hits powerpc, scsi and block. So who gets to
> merge this? Jens? James? Paul?
>
> Me, I guess ;)
As Paul is on holidays, please take it. The PS3 storage driver core support is
already in mainline, but the actual drivers aren't, as Paul was waiting for
acks from the maintainers.
BTW, do you prefer incremental patches for the comments below, or an update of
the full patchset?
> > +#define KERNEL_SECTOR_SIZE 512
>
> Sigh. We have at least ten separate definitions of SECTOR_SIZE< none of
> them in the right place. Cleanup opportunity for someone.
Will replace by 512 resp. >> 9, as per Jens' suggestion.
> > +struct ps3disk_private {
> > + spinlock_t lock; /* Request queue spinlock */
> > + struct request_queue *queue;
> > + struct gendisk *gendisk;
> > + unsigned int blocking_factor;
> > + struct request *req;
> > + u64 raw_capacity;
> > + unsigned char model[ATA_ID_PROD_LEN+1];
> > +};
> > +#define ps3disk_priv(dev) ((dev)->sbd.core.driver_data)
>
> hm, this code has "ata" all over it and actually uses a libata #define (at
> least) but there is no Kconfig dependency on ATA. Fair enough, I guess,
> but a bit funny-looking.
I needed the definitions just for drive identification.
> > +static void ps3disk_scatter_gather(struct ps3_storage_device *dev,
> > + struct request *req, int gather)
> > +{
> > + unsigned int sectors = 0, offset = 0;
> Local variable `sectors' doesn't do anything.
Good catch!
> > +static int ps3disk_submit_request_sg(struct ps3_storage_device *dev,
> > + struct request *req)
> > +{
> > + struct ps3disk_private *priv = ps3disk_priv(dev);
> > + int write = rq_data_dir(req), res;
> > + const char *op = write ? "write" : "read";
> > + u64 start_sector, sectors;
> > + unsigned int region_id = dev->regions[dev->region_idx].id;
>
> So we're ignoring the sector_t stuff. I guess it's 64-bit only. Still, it
> might be nice to use sector_t for consistency, readability and possible
> future 32-bitness?
I used u64 as they're directly passed to hypervisor calls and that's what the
hypervisor calls take.
BTW, Cell will never support a 32-bit kernel, as lots of on-chip stuff lies
outside the 32-bit address space.
> > +#ifdef DEBUG
> > + unsigned int n = 0;
> > + struct bio *bio;
> > + rq_for_each_bio(bio, req)
> > + n++;
>
> I'm surprised that the block core doesn't have a helper to count the number
> of bios in a request.
>
> Please prefer to put a blank line between end-of-locals and start-of-code.
Done.
> > + dev_dbg(&dev->sbd.core,
> > + "%s:%u: %s req has %u bios for %lu sectors %lu hard sectors\n",
> > + __func__, __LINE__, op, n, req->nr_sectors,
> > + req->hard_nr_sectors);
> > +#endif
> > +
> > + start_sector = req->sector*priv->blocking_factor;
> > + sectors = req->nr_sectors*priv->blocking_factor;
>
> s/*/ * /. checkpatch missed this.
Will change (yes, checkpatch missed it).
> I suspect you didn't run cehckpatch anyway.
I did ;-)
> Please run checkpatch.
All it complains about is (bogus) <asm/firmware.h> and a single too long line I
don't want to break.
> > +/* ATA helpers copied from drivers/ata/libata-core.c */
>
> ooh, bad person.
I didn't have much choice, as most of it was static and I don't need the full
libata core anyway.
If I would factor it out, any good suggestion where to put the factored out
code?
> > + ps3disk_identify(dev);
>
> ps3disk_identify() can return an error?
Sending storage commands may fail, but being unable to identify the disk isn't
fatal, as it's just for informational purposes.
> > +static int ps3disk_remove(struct ps3_system_bus_device *_dev)
> > +{
> > + struct ps3_storage_device *dev = to_ps3_storage_device(&_dev->core);
> > + struct ps3disk_private *priv = ps3disk_priv(dev);
> > +
> > + __clear_bit(priv->gendisk->first_minor / PS3DISK_MINORS,
> > + &ps3disk_mask);
>
> I see no locking here which makes this __clear_bit and the above __set_bit
> non-racy?
Were .probe()/.remove() made concurrent again? I thought that idea was dropped
because it caused too many problems?
> > + kfree(priv);
> > + ps3disk_priv(dev) = NULL;
>
> I suspect this nulling here will just hide any bugs? If we're going to
> write anything there, probably 0xdeadbeef would be better?
Hmm, earlier reviewers explicitly asked for putting it there...
Thanks for your comments!
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/
Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619
^ permalink raw reply
* Re: [patch 1/3] ps3: Disk Storage Driver
From: Geert Uytterhoeven @ 2007-07-19 7:15 UTC (permalink / raw)
To: Jens Axboe
Cc: James E.J. Bottomley, linux-scsi, linux-kernel, Alessandro Rubini,
linuxppc-dev, Paul Mackerras, Andrew Morton
In-Reply-To: <20070719053302.GO11657@kernel.dk>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1215 bytes --]
On Thu, 19 Jul 2007, Jens Axboe wrote:
> On Wed, Jul 18 2007, Andrew Morton wrote:
> > On Mon, 16 Jul 2007 18:15:40 +0200
> > Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
> >
> > > From: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
> > >
> > > Add a Disk Storage Driver for the PS3:
> >
> > Your patchset significantly hits powerpc, scsi and block. So who gets to
> > merge this? Jens? James? Paul?
> >
> > Me, I guess ;)
>
> I think Paul was going to take it, or at least Geert hinted as such.
Yep, but as I heard Paul is on holidays, I was just going to send it to Andrew
anyway.
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/
Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619
^ permalink raw reply
* Re: [patch 0/3] PS3 Storage Drivers for 2.6.23, take 5
From: Alessandro Rubini @ 2007-07-19 6:40 UTC (permalink / raw)
To: Geert.Uytterhoeven
Cc: axboe, James.Bottomley, linux-scsi, linux-kernel, linuxppc-dev,
paulus
In-Reply-To: <20070716161539.075822000@pademelon.sonytel.be>
Hello.
> I didn't hear anything from the misc device maintainer (for the FLASH ROM
> Storage Driver).
Actually, I am not acting as a maintainer. I'm not active enough nor
up to date with all the structure of kernel maintainance. So please
don't wait for me.
Actually, I tried a pair of times to have my name removed from the
MAINTAINERS file over the years without success. Actually, I didn't
care a lot because nobody relly used that entry. I think it's time for
me to learn how to do it in the proper way.
Regards
/alessandro
^ permalink raw reply
* Re: [PATCH 2/2] fix showing xmon help
From: Milton Miller @ 2007-07-19 5:50 UTC (permalink / raw)
To: Andreas Schwab; +Cc: ppcdev, Paul Mackerras
In-Reply-To: <je1wf5bshh.fsf@sykes.suse.de>
On Jul 18, 2007, at 11:12 AM, Andreas Schwab wrote:
> Milton Miller <miltonm@bga.com> writes:
>
>>> case '?':
>>> - printf(help_string);
>>> + xmon_puts(help_string);
>>> break;
>>>
>>
>> nonstdio.h #defines printf to xmon_printf. Please add a similar line
>> for puts, and use the define here. (It will avoid an unnecessary
>> difference with the user space version).
>
> User space puts add a newline, which this xmon_puts doesn't.
>
> Andreas.
Good point. This should be xmon_fputs #defined to fputs.
milton
^ permalink raw reply
* Re: [PATCH 10/61] bootwrapper: flatdevtree fixes
From: Milton Miller @ 2007-07-19 5:45 UTC (permalink / raw)
To: Scott Wood; +Cc: ppcdev, Paul Mackerras, David Gibson
In-Reply-To: <469E6241.5090604@freescale.com>
On Jul 18, 2007, at 1:56 PM, Scott Wood wrote:
> Milton Miller wrote:
>> In-Reply-To: <20070718013137.GA15217@ld0162-tx32.am.freescale.net>
>> On Wed Jul 18 11:33:08 EST 2007, Scott Wood wrote:
>>> 1. ft_create_node was returning the internal pointer rather than a
>>> phandle.
>>> 2. ft_find_device_rel was treating lookups relative to root as an
>>> error.
>> No, it is treating lookups relative to NULL as an error.
>> Your patch changes it to treat lookups relative to the NULL phandle
>> as relative to root.
>
> Other parts of the code (such as ft_create_node) already do that.
> What's wrong with it?
Well, that behavior may be ok, but the change log should say that is
the behavior you are changing.
Ie I would record 2 as:
When ft_find_device_rel is asked to find relative to NULL, find
realative to root instead of returning an error.
or:
ft_find_device_rel now treats lookups relative to NULL as relative to
the root instead of erroring out.
My reading would say that lookups relative to the phandle of root would
have workied before, no?
>
>> I've no objections to the other part, can you split these?
>
> No, because change #1 won't work without change #2.
Huh?
I can see how changing a routine to return an external not internal
pointer is necessary before changing a caller creates a dependancy for
2 on 1, but why does it require lookups relative to the phandle NULL be
treated as lookups relative to the root?
milton
^ permalink raw reply
* Re: [patch 1/3] ps3: Disk Storage Driver
From: Jens Axboe @ 2007-07-19 5:33 UTC (permalink / raw)
To: Andrew Morton
Cc: James E.J. Bottomley, linux-scsi, linux-kernel, Alessandro Rubini,
linuxppc-dev, Paul Mackerras, Geert Uytterhoeven
In-Reply-To: <20070718163637.3f0e0164.akpm@linux-foundation.org>
On Wed, Jul 18 2007, Andrew Morton wrote:
> On Mon, 16 Jul 2007 18:15:40 +0200
> Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
>
> > From: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
> >
> > Add a Disk Storage Driver for the PS3:
>
> Your patchset significantly hits powerpc, scsi and block. So who gets to
> merge this? Jens? James? Paul?
>
> Me, I guess ;)
I think Paul was going to take it, or at least Geert hinted as such.
> > +#define PS3DISK_MAX_DISKS 16
> > +#define PS3DISK_MINORS 16
> > +
> > +#define KERNEL_SECTOR_SIZE 512
>
> Sigh. We have at least ten separate definitions of SECTOR_SIZE< none of
> them in the right place. Cleanup opportunity for someone.
Indeed, it's universally 512 or << 9, just use that...
> > +#ifdef DEBUG
> > + unsigned int n = 0;
> > + struct bio *bio;
> > + rq_for_each_bio(bio, req)
> > + n++;
>
> I'm surprised that the block core doesn't have a helper to count the number
> of bios in a request.
What would be the point of such a helper? I've never seen a need for it.
Geert uses it as debug code here, but the fact is that the number of
bios in a request is a pretty pointless number. It doesn't tell you
anything. There's no 1:1 mapping between bios and segments (or anything
else for that matter), so the exercise is purely pointless.
--
Jens Axboe
^ permalink raw reply
* Re: Powerpc - Include pagemap.h in asm/powerpc/tlb.h
From: Andrew Morton @ 2007-07-19 5:30 UTC (permalink / raw)
To: Mathieu Desnoyers; +Cc: linuxppc-dev, Paul Mackerras, linux-kernel
In-Reply-To: <20070714012033.GC31649@Krystal>
On Fri, 13 Jul 2007 21:20:33 -0400 Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote:
> Powerpc - Include pagemap.h in asm/powerpc/tlb.h
>
> Fixes this powerpc build error in 2.6.22-rc6-mm1 for powerpc 64 :
>
> In file included from include2/asm/tlb.h:60,
> from /home/compudj/git/linux-2.6-lttng/arch/powerpc/mm/init_64.
> c:56:
> /home/compudj/git/linux-2.6-lttng/include/asm-generic/tlb.h: In function 'tlb_fl
> ush_mmu':
> /home/compudj/git/linux-2.6-lttng/include/asm-generic/tlb.h:76: error: implicit
> declaration of function 'release_pages'
> /home/compudj/git/linux-2.6-lttng/include/asm-generic/tlb.h: In function 'tlb_re
> move_page':
> /home/compudj/git/linux-2.6-lttng/include/asm-generic/tlb.h:105: error: implicit
> declaration of function 'page_cache_release'
You have some wordwrapping going on there.
> make[2]: *** [arch/powerpc/mm/init_64.o] Error 1
>
> release_pages is declared in linux/pagemap.h, but cannot be included in
> linux/swap.h because of a sparc related comment:
>
> /* only sparc can not include linux/pagemap.h in this file
> * so leave page_cache_release and release_pages undeclared... */
> #define free_page_and_swap_cache(page) \
> page_cache_release(page)
> #define free_pages_and_swap_cache(pages, nr) \
> release_pages((pages), (nr), 0);
It's always a worry when this happens. What change made us need this
inclusion? How come you're hitting it but I (and test.kernel.org, at least)
did not? How come so few other architectures include pagemap.h from
asm/tlb.h? Why do header files get into such a mess?
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
> CC: linuxppc-dev@ozlabs.org
> CC: Paul Mackerras <paulus@samba.org>
> ---
> include/asm-powerpc/tlb.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> Index: linux-2.6-lttng/include/asm-powerpc/tlb.h
> ===================================================================
> --- linux-2.6-lttng.orig/include/asm-powerpc/tlb.h 2007-07-13 11:30:54.000000000 -0400
> +++ linux-2.6-lttng/include/asm-powerpc/tlb.h 2007-07-13 11:31:22.000000000 -0400
> @@ -23,6 +23,8 @@
> #include <asm/mmu.h>
> #endif
>
> +#include <linux/pagemap.h>
> +
> struct mmu_gather;
>
Oh well. I queued it up for someone else to worry over ;)
^ permalink raw reply
* Re: [PATCH 02/11] dtc: move declaration of yyerror
From: David Gibson @ 2007-07-19 5:02 UTC (permalink / raw)
To: Milton Miller; +Cc: linuxppc-dev, Jon Loeliger
In-Reply-To: <dtc-5-02.miltonm@bga.com>
On Sat, Jul 07, 2007 at 01:18:47AM -0500, Milton Miller wrote:
> yyerror() is used by both dtc-parser.y and dtc-lexer.l, so move
> the declaration to srcpos.h.
>
> Signed-off-by: Milton Miller <miltonm@bga.com>
> ---
> eliminates implicit declaration warning.
Eck. This wasn't necessary: that part of dtc-parser.y is included
into dtc-parser.tab.h which in turn is included into dtc-lexer.l.
srcpos.h is supposed to be exclusively for, well, the source position
stuff.
Please revert.
> Index: dtc/dtc-parser.y
> ===================================================================
> --- dtc.orig/dtc-parser.y 2007-06-14 23:16:18.000000000 -0500
> +++ dtc/dtc-parser.y 2007-06-14 23:16:25.000000000 -0500
> @@ -26,7 +26,6 @@
> #include "srcpos.h"
>
> int yylex(void);
> -void yyerror(char const *);
> cell_t cell_from_string(char *s, unsigned int base);
>
> extern struct boot_info *the_boot_info;
> Index: dtc/srcpos.h
> ===================================================================
> --- dtc.orig/srcpos.h 2007-06-14 23:16:18.000000000 -0500
> +++ dtc/srcpos.h 2007-06-14 23:16:25.000000000 -0500
> @@ -62,6 +62,7 @@ typedef struct YYLTYPE {
>
>
>
> +extern void yyerror(char const *);
>
> extern int srcpos_filenum;
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply
* Re: [PATCH 2/15] boot: record header bytes in gunzip_start
From: Milton Miller @ 2007-07-19 4:46 UTC (permalink / raw)
To: David Gibson; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <20070719021138.GE20458@localhost.localdomain>
On Jul 18, 2007, at 9:11 PM, David Gibson wrote:
> On Tue, Jul 10, 2007 at 05:08:05PM -0500, Milton Miller wrote:
>>
>> Record the number of header bytes skipped in the total bytes read
>> field.
>>
>> This is needed for the initramfs parsing code to find the end of the
>> zip file.
>>
>> Signed-off-by: Milton Miller <miltonm@bga.com>
>
> Ok... I assume you've checked that this is actually the correct
> semantics for that field of the zlib structure?
From looking at the comments in include/linux/zlib.h, I think its
consistent. It talks about looking at that field for statistics and
for the application to record how many bytes were read during
compression. It mentions it brifely in the inflateSync to show how
many bytes have been processed. So I say its consistent to say we
have processed the header bytes in the input file.
It appears to be for recording the number of bytes processed over all
iterations.
I dig into zlib to see if this matches the beahvior of decompressing a
zlib header, but expect it would match.
milton
^ permalink raw reply
* Re: [PATCH 3/15] boot: simplfy gunzip_finish
From: David Gibson @ 2007-07-19 4:31 UTC (permalink / raw)
To: Milton Miller; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <323b556138e9e745eb4fc6327faf7774@bga.com>
On Wed, Jul 18, 2007 at 11:01:37PM -0500, Milton Miller wrote:
>
> On Jul 18, 2007, at 9:39 PM, David Gibson wrote:
>
> > On Tue, Jul 10, 2007 at 05:08:32PM -0500, Milton Miller wrote:
> >>
> >> Call gunzip_partial to calculate the remaining length and copy the
> >> data to the user buffer. This makes it shorter and reduces
> >> duplication.
> >>
> >> Signed-off-by: Milton Miller <miltonm@bga.com>
> >
> > Hrm... I guess this is sufficient. Since we'll shortly be blowing
> > away the state anyway, the lack of call to inflateEnd doesn't really
> > matter. Originally I was going to make this function call fatal() if
> > the destination didn't have enough space to contain the decompressed
> > tail of the image, but obviously I decided aginst that.
> >
> > But, if you're going to do this, you might as well get rid of
> > gunzip_finish() entirely, and have the callers using gunzip_partial
> > instead.
>
> Huh? I stilll call inflateEnd if s.workspace is not NULL, I just
> don't duplicate gunzip_partial in gunzip_finish. Calling inflateEnd
Oh, yes, oops. Missed the couple of lines there that didn't have a
'-' in front.
> will mean a future call would return an error in the compressed case
> instead of the behavior in the non-compressed case that changes with
> this patch from get same data again to get further data that may exist
> after the patch.
>
> Yes, one can argue that we don't need to call inflateEnd, but I didn't
> make that change in this patch.
>
> Or were you suggesting that we really don't need the finish function?
Well, we probably don't. But for the time being.
Acked-by: David Gibson <david@gibson.dropbear.id.au>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply
* Re: [PATCH 3/15] boot: simplfy gunzip_finish
From: Milton Miller @ 2007-07-19 4:01 UTC (permalink / raw)
To: David Gibson; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <20070719023917.GF20458@localhost.localdomain>
On Jul 18, 2007, at 9:39 PM, David Gibson wrote:
> On Tue, Jul 10, 2007 at 05:08:32PM -0500, Milton Miller wrote:
>>
>> Call gunzip_partial to calculate the remaining length and copy the
>> data to the user buffer. This makes it shorter and reduces
>> duplication.
>>
>> Signed-off-by: Milton Miller <miltonm@bga.com>
>
> Hrm... I guess this is sufficient. Since we'll shortly be blowing
> away the state anyway, the lack of call to inflateEnd doesn't really
> matter. Originally I was going to make this function call fatal() if
> the destination didn't have enough space to contain the decompressed
> tail of the image, but obviously I decided aginst that.
>
> But, if you're going to do this, you might as well get rid of
> gunzip_finish() entirely, and have the callers using gunzip_partial
> instead.
Huh? I stilll call inflateEnd if s.workspace is not NULL, I just
don't duplicate gunzip_partial in gunzip_finish. Calling inflateEnd
will mean a future call would return an error in the compressed case
instead of the behavior in the non-compressed case that changes with
this patch from get same data again to get further data that may exist
after the patch.
Yes, one can argue that we don't need to call inflateEnd, but I didn't
make that change in this patch.
Or were you suggesting that we really don't need the finish function?
milton
^ permalink raw reply
* Re: [PATCH] fix future firmware feature fixups function failure
From: Michael Neuling @ 2007-07-19 2:55 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: paulus, linuxppc-dev
In-Reply-To: <200707190054.17754.arnd@arndb.de>
> On Wednesday 18 July 2007, Michael Neuling wrote:
> > Move firmware feature initialisation from pSeries_init_early to the
> > earlier pSeries_probe_hypertas so they are initialised before firmware
> > feature fixups are applied.
> >=20
> > Currently firmware feature sections are only used for iSeries which
> > initialises the these features much earlier. =A0This is a bug in waiting
> > on pSeries.
> >=20
> > Also adds some whitespace fixups.
> >=20
> > Signed-off-by: Michael Neuling <mikey@neuling.org>
>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
>
> Haven't tested it myself, but it certainly looks good to me. It does
> require reverting your previous patch though, are you submitting the
> reversal patch as well?
Yep, this got reverted yesterday in
826ea8f22cf612d534f33c492c98f7895043bfd1
Mikey
^ permalink raw reply
* Re: [PATCH 3/15] boot: simplfy gunzip_finish
From: David Gibson @ 2007-07-19 2:39 UTC (permalink / raw)
To: Milton Miller; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <boot-6-03.miltonm@bga.com>
On Tue, Jul 10, 2007 at 05:08:32PM -0500, Milton Miller wrote:
>
> Call gunzip_partial to calculate the remaining length and copy the
> data to the user buffer. This makes it shorter and reduces
> duplication.
>
> Signed-off-by: Milton Miller <miltonm@bga.com>
Hrm... I guess this is sufficient. Since we'll shortly be blowing
away the state anyway, the lack of call to inflateEnd doesn't really
matter. Originally I was going to make this function call fatal() if
the destination didn't have enough space to contain the decompressed
tail of the image, but obviously I decided aginst that.
But, if you're going to do this, you might as well get rid of
gunzip_finish() entirely, and have the callers using gunzip_partial
instead.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply
* Re: [PATCH 2/15] boot: record header bytes in gunzip_start
From: David Gibson @ 2007-07-19 2:11 UTC (permalink / raw)
To: Milton Miller; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <boot-6-02.miltonm@bga.com>
On Tue, Jul 10, 2007 at 05:08:05PM -0500, Milton Miller wrote:
>
> Record the number of header bytes skipped in the total bytes read field.
>
> This is needed for the initramfs parsing code to find the end of the zip file.
>
> Signed-off-by: Milton Miller <miltonm@bga.com>
Ok... I assume you've checked that this is actually the correct
semantics for that field of the zlib structure?
> Index: work.git/arch/powerpc/boot/gunzip_util.c
> ===================================================================
> --- work.git.orig/arch/powerpc/boot/gunzip_util.c 2007-07-10 03:44:41.000000000 -0500
> +++ work.git/arch/powerpc/boot/gunzip_util.c 2007-07-10 03:47:35.000000000 -0500
> @@ -78,6 +78,7 @@ void gunzip_start(struct gunzip_state *s
> fatal("inflateInit2 returned %d\n\r", r);
> }
>
> + state->s.total_in = hdrlen;
> state->s.next_in = src + hdrlen;
> state->s.avail_in = srclen - hdrlen;
> }
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply
* Re: [PATCH 1/15] boot: find initrd location from device-tree
From: David Gibson @ 2007-07-19 2:10 UTC (permalink / raw)
To: Milton Miller; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <boot-6-01.miltonm@bga.com>
On Tue, Jul 10, 2007 at 05:07:38PM -0500, Milton Miller wrote:
> Some platforms have a boot agent that can create or modify properties in
> the device-tree and load images into memory. Provide a helper to set
> loader_info used by prep_initrd().
>
> Signed-off-by: Milton Miller <miltonm@bga.com>
Looks reasonable.
Acked-by: David Gibson <david@gibson.dropbear.id.au>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply
* Re: [PATCH 17/61] bootwrapper: Add 8xx support.
From: David Gibson @ 2007-07-19 1:32 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev
In-Reply-To: <469E3C11.1090809@freescale.com>
On Wed, Jul 18, 2007 at 11:13:05AM -0500, Scott Wood wrote:
> David Gibson wrote:
> > On Tue, Jul 17, 2007 at 08:33:18PM -0500, Scott Wood wrote:
> > [snip]
> >
> >>diff --git a/arch/powerpc/boot/cuboot-8xx.c b/arch/powerpc/boot/cuboot-8xx.c
> >>new file mode 100644
> >>index 0000000..35476a0
> >>--- /dev/null
> >>+++ b/arch/powerpc/boot/cuboot-8xx.c
> >>@@ -0,0 +1,47 @@
> >>+/*
> >>+ * Old U-boot compatibility for 8xx
> >>+ *
> >>+ * Author: Scott Wood <scottwood@freescale.com>
> >>+ *
> >>+ * Copyright (c) 2007 Freescale Semiconductor, Inc.
> >>+ *
> >>+ * This program is free software; you can redistribute it and/or modify it
> >>+ * under the terms of the GNU General Public License version 2 as published
> >>+ * by the Free Software Foundation.
> >>+ */
> >>+
> >>+#include "ops.h"
> >>+#include "stdio.h"
> >>+#include "cuboot.h"
> >>+
> >>+#define TARGET_8xx
> >>+#define TARGET_HAS_ETH1
> >
> > ^^^^^^^^^^^^^^^
> > Is that really true for all 8xx boards?
>
> Well, no. Even on a given board, it depends on the version of u-boot.
>
> There's nothing after enet1addr that the bootwrapper cares about,
> though, so the only harm is if the device tree has a second network
> interface but u-boot doesn't know about it, and the bootwrapper ends up
> pulling in junk rather than leaving zeroes.
That sounds like a terribly fragile way of handling things.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply
* Re: [PATCH 05/61] bootwrapper: Change cuImage .gitignore entry to cuImage.*
From: David Gibson @ 2007-07-19 1:32 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev
In-Reply-To: <469E3B27.6040406@freescale.com>
On Wed, Jul 18, 2007 at 11:09:11AM -0500, Scott Wood wrote:
> David Gibson wrote:
> > On Tue, Jul 17, 2007 at 08:33:01PM -0500, Scott Wood wrote:
> >
> >>Signed-off-by: Scott Wood <scottwood@freescale.com>
> >>---
> >> arch/powerpc/boot/.gitignore | 4 +---
> >> 1 files changed, 1 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/arch/powerpc/boot/.gitignore b/arch/powerpc/boot/.gitignore
> >>index eec7af7..3270335 100644
> >>--- a/arch/powerpc/boot/.gitignore
> >>+++ b/arch/powerpc/boot/.gitignore
> >>@@ -18,9 +18,7 @@ kernel-vmlinux.strip.c
> >> kernel-vmlinux.strip.gz
> >> mktree
> >> uImage
> >>-cuImage
> >>-cuImage.bin.gz
> >>-cuImage.elf
> >>+cuImage.*
> >> zImage
> >> zImage.chrp
> >> zImage.coff
> >
> >
> > Err... your new wildcard won't match plain 'cuImage'.
>
> When is plain "cuImage" ever generated anymore?
Oh, yes...
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply
* Re: [PATCH] make powerpc pci compile again
From: Arnd Bergmann @ 2007-07-19 0:23 UTC (permalink / raw)
To: linuxppc-dev, michael; +Cc: Johannes Berg, Paul Mackerras
In-Reply-To: <1184803727.9798.17.camel@concordia.ozlabs.ibm.com>
On Thursday 19 July 2007, Michael Ellerman wrote:
> Your patch is correct, but I think Tony has already sent it.
>
yes, it's already upstream as of Wed Jul 18 11:03:55 2007 +1000
Arnd <><
^ permalink raw reply
* Re: [PATCH 27/61] cpm2: Fix whitespace in cpm2_clk_setup().
From: Arnd Bergmann @ 2007-07-19 0:21 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20070718013534.GY15238@ld0162-tx32.am.freescale.net>
On Wednesday 18 July 2007, Scott Wood wrote:
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
> =A0arch/powerpc/sysdev/cpm2_common.c | =A0 =A02 +-
> =A01 files changed, 1 insertions(+), 1 deletions(-)
>=20
> diff --git a/arch/powerpc/sysdev/cpm2_common.c b/arch/powerpc/sysdev/cpm2=
_common.c
> index 4e840bd..7e3a983 100644
> --- a/arch/powerpc/sysdev/cpm2_common.c
> +++ b/arch/powerpc/sysdev/cpm2_common.c
> @@ -233,7 +233,7 @@ int cpm2_clk_setup(enum cpm_clk_target target, int cl=
ock, int mode)
> =A0=A0=A0=A0=A0=A0=A0=A0}
> =A0
> =A0=A0=A0=A0=A0=A0=A0=A0if (mode =3D=3D CPM_CLK_RX)
> -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0shift +=3D3;
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0shift +=3D 3;
> =A0
> =A0=A0=A0=A0=A0=A0=A0=A0for (i =3D 0; i < ARRAY_SIZE(clk_map); i++) {
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0if (clk_map[i][0] =3D=3D =
target && clk_map[i][1] =3D=3D clock) {
While all your 61 patches look really good to me in general, I think you're=
driving
the 'one patch per logical change' mantra a little too far. E.g. a number o=
f them
could simply have been consolidated under 'fix obvious coding style violati=
ons in
arch/powerpc'.
Having patches that fix a single whitespace bug only causes extra work for =
both
the author and the reviewer without much benefit.
Arnd <><
^ permalink raw reply
* Re: [PATCH 37/61] cpm2: Rework 8272ads initialization, and eliminate init_fcc_ioports().
From: Arnd Bergmann @ 2007-07-19 0:14 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <200707190204.44825.arnd@arndb.de>
On Thursday 19 July 2007, Arnd Bergmann wrote:
> I guess you considered moving such tables into the device tree at some point,
> which doesn't seem that difficult at all to an ignorant reviewer like me.
>
> What's the reasoning for putting it into the platform code after all?
>
Ok, I think I found the answer myself: the table is for stuff that Linux
needs to set up, not to describe stuff that is already set up at the time
the device tree is parsed, right?
Arnd <><
^ permalink raw reply
* Re: [PATCH] make powerpc pci compile again
From: Michael Ellerman @ 2007-07-19 0:08 UTC (permalink / raw)
To: Johannes Berg; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <1184779608.8125.2.camel@johannes.berg>
[-- Attachment #1: Type: text/plain, Size: 1321 bytes --]
On Wed, 2007-07-18 at 19:26 +0200, Johannes Berg wrote:
> Looks like we got a new declaration in the generic header... This fixes
> compile but I don't know whether it's correct.
We got a new declaration, along with updated C code in pci_64.c and
pci_32.c, but when that was merged with the creation of pci-common.c it
all went fubar.
Your patch is correct, but I think Tony has already sent it.
cheers
> --- linux-2.6-git.orig/arch/powerpc/kernel/pci-common.c 2007-07-17 19:11:57.519997066 +0200
> +++ linux-2.6-git/arch/powerpc/kernel/pci-common.c 2007-07-17 19:12:14.820997066 +0200
> @@ -156,10 +156,12 @@ static DEVICE_ATTR(devspec, S_IRUGO, pci
> #endif /* CONFIG_PPC_OF */
>
> /* Add sysfs properties */
> -void pcibios_add_platform_entries(struct pci_dev *pdev)
> +int pcibios_add_platform_entries(struct pci_dev *pdev)
> {
> #ifdef CONFIG_PPC_OF
> - device_create_file(&pdev->dev, &dev_attr_devspec);
> + return device_create_file(&pdev->dev, &dev_attr_devspec);
> +#else
> + return 0;
> #endif /* CONFIG_PPC_OF */
> }
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [PATCH 37/61] cpm2: Rework 8272ads initialization, and eliminate init_fcc_ioports().
From: Arnd Bergmann @ 2007-07-19 0:04 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20070718013548.GI15238@ld0162-tx32.am.freescale.net>
On Wednesday 18 July 2007, Scott Wood wrote:
> +static struct cpm_pin mpc8272ads_pins[] =3D {
> +=A0=A0=A0=A0=A0=A0=A0/* SCC1 */
> +=A0=A0=A0=A0=A0=A0=A0{3, 30, CPM_PIN_OUTPUT | CPM_PIN_SECONDARY},
> +=A0=A0=A0=A0=A0=A0=A0{3, 31, CPM_PIN_INPUT | CPM_PIN_PRIMARY},
> +
> +=A0=A0=A0=A0=A0=A0=A0/* SCC4 */
> +=A0=A0=A0=A0=A0=A0=A0{3, 21, CPM_PIN_OUTPUT | CPM_PIN_PRIMARY},
> +=A0=A0=A0=A0=A0=A0=A0{3, 22, CPM_PIN_INPUT | CPM_PIN_PRIMARY},
> +
> +=A0=A0=A0=A0=A0=A0=A0/* FCC1 */
> +=A0=A0=A0=A0=A0=A0=A0{0, 14, CPM_PIN_INPUT | CPM_PIN_PRIMARY},
> +=A0=A0=A0=A0=A0=A0=A0{0, 15, CPM_PIN_INPUT | CPM_PIN_PRIMARY},
> +=A0=A0=A0=A0=A0=A0=A0{0, 16, CPM_PIN_INPUT | CPM_PIN_PRIMARY},
> +=A0=A0=A0=A0=A0=A0=A0{0, 17, CPM_PIN_INPUT | CPM_PIN_PRIMARY},
I guess you considered moving such tables into the device tree at some poin=
t,
which doesn't seem that difficult at all to an ignorant reviewer like me.
What's the reasoning for putting it into the platform code after all?
Arnd <><
^ permalink raw reply
* Re: [PATCH] Treat ISI faults as read faults on classic 32-bit PowerPC
From: Paul Mackerras @ 2007-07-19 0:00 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev, Johannes Berg
In-Reply-To: <11847726193856-git-send-email-segher@kernel.crashing.org>
Segher Boessenkool writes:
> Some old software on ppc32 executes from pages it hasn't marked
> executable. Since "classic" hardware doesn't distinguish between
> execute and read accesses, the do_page_fault() code shouldn't
> either. This makes glibc-2.2 work again on such hardware.
>
> Signed-off-by: Segher Boessenkool <segher@kernel.crashing.org>
> Cc: Scott Wood <scottwood@freescale.com>
> Cc: Johannes Berg <johannes@sipsolutions.net>
> ---
> [Resend again, ozlabs' greylisting doesn't like me at all.]
>
> Tested by Scott on 32-bit, glibc-2.2.5 and glibc-2.3.3 (no new
> failures and problem solved), and by Johannes on his glibc-2.4
> "---p" testcase. Could use testing on ppc64 and BookE too, for
> good measure.
Hmmm. The dangling else clauses are pretty gross, and in fact we have
the same problem on POWER3 and RS64 processors (to be fair, we had
the problem before and didn't notice, but we should still fix it).
How about this instead? Could people test it please? (Note that
CPU_FTR_NOEXECUTE is 0 in 32-bit kernels.)
Paul.
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 0ece513..99c3093 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -283,7 +283,13 @@ good_area:
/* protection fault */
if (error_code & DSISR_PROTFAULT)
goto bad_area;
- if (!(vma->vm_flags & VM_EXEC))
+ /*
+ * Allow execution from readable areas if the MMU does not
+ * provide separate controls over reading and executing.
+ */
+ if (!(vma->vm_flags & VM_EXEC) &&
+ (cpu_has_feature(CPU_FTR_NOEXECUTE) ||
+ !(vma->vm_flags & (VM_READ | VM_WRITE))))
goto bad_area;
#else
pte_t *ptep;
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox