Re: RFC: android logger feedback request

2011-12-28 Thread Andrew Morton 
On Wed, 21 Dec 2011 14:59:15 -0800
Tim Bird tim.b...@am.sony.com wrote:

 Hi all,
 
 I'm looking for feedback on the Android logger code,

The code looks nice.


 ...

 +/* logger_offset - returns index 'n' into the log via (optimized) modulus */
 +#define logger_offset(n) ((n)  (log-size - 1))
 +

This macro depends upon the presence of a local variable called log
and is therefore all stinky.  Pass log in as an arg and convert it to
a regular C function.


 ...

 +/*
 + * get_entry_len - Grabs the length of the payload of the next entry starting
 + * from 'off'.
 + *
 + * Caller needs to hold log-mutex.
 + */
 +static __u32 get_entry_len(struct logger_log *log, size_t off)
 +{
 + __u16 val;
 +
 + switch (log-size - off) {
 + case 1:
 + memcpy(val, log-buffer + off, 1);
 + memcpy(((char *) val) + 1, log-buffer, 1);

So numbers in the buffer are in host endian order.  That's worth
capturing in a comment somewhere.

There must be a way of doing the above more neatly, using
log-buffer[off] and log-buffer[0].  Perhaps using
include/linux/unaligned/packed_struct.h.

 + break;
 + default:
 + memcpy(val, log-buffer + off, 2);

get_unaligned()

 + }
 +
 + return sizeof(struct logger_entry) + val;
 +}
 +

 ...

 +static ssize_t logger_read(struct file *file, char __user *buf,
 +size_t count, loff_t *pos)
 +{
 + struct logger_reader *reader = file-private_data;
 + struct logger_log *log = reader-log;
 + ssize_t ret;
 + DEFINE_WAIT(wait);
 +
 +start:
 + while (1) {
 + prepare_to_wait(log-wq, wait, TASK_INTERRUPTIBLE);
 +
 + mutex_lock(log-mutex);

If mutex_lock() had to wait for the mutex, it will return in state
TASK_RUNNING, thus rubbing out the effects of prepare_to_wait().  We'll
just loop again so this is a benign buglet.

Could we lose a wakeup by running prepaer_to_wait() outside the lock? 
I don't think so, but I stopped thinking about it when I saw the above
bug.  These two lines should be switched around.

 + ret = (log-w_off == reader-r_off);
 + mutex_unlock(log-mutex);
 + if (!ret)
 + break;
 +
 + if (file-f_flags  O_NONBLOCK) {
 + ret = -EAGAIN;
 + break;
 + }
 +
 + if (signal_pending(current)) {
 + ret = -EINTR;
 + break;
 + }
 +
 + schedule();
 + }
 +
 + finish_wait(log-wq, wait);
 + if (ret)
 + return ret;
 +
 + mutex_lock(log-mutex);
 +
 + /* is there still something to read or did we race? */
 + if (unlikely(log-w_off == reader-r_off)) {
 + mutex_unlock(log-mutex);
 + goto start;
 + }
 +
 + /* get the size of the next entry */
 + ret = get_entry_len(log, reader-r_off);
 + if (count  ret) {
 + ret = -EINVAL;
 + goto out;
 + }
 +
 + /* get exactly one entry from the log */
 + ret = do_read_log_to_user(log, reader, buf, ret);
 +
 +out:
 + mutex_unlock(log-mutex);
 +
 + return ret;
 +}
 +

 ...

 +/*
 + * clock_interval - is a  c  b in mod-space? Put another way, does the line
 + * from a to b cross c?
 + */

This is where my little brain started to hurt.  I assume it's correct ;)

 +static inline int clock_interval(size_t a, size_t b, size_t c)
 +{
 + if (b  a) {
 + if (a  c || b = c)
 + return 1;
 + } else {
 + if (a  c  b = c)
 + return 1;
 + }
 +
 + return 0;
 +}

The explicit inline(s) are increaseingly old-fashioned.  gcc is good
about this now.


 ...

 +static ssize_t do_write_log_from_user(struct logger_log *log,
 +   const void __user *buf, size_t count)
 +{
 + size_t len;
 +
 + len = min(count, log-size - log-w_off);
 + if (len  copy_from_user(log-buffer + log-w_off, buf, len))
 + return -EFAULT;
 +
 + if (count != len)
 + if (copy_from_user(log-buffer, buf + len, count - len))
 + return -EFAULT;

Is it correct to simply return here without updating -w_off? 
We've just copied len bytes in from userspace.

 + log-w_off = logger_offset(log-w_off + count);
 +
 + return count;
 +}
 +
 +/*
 + * logger_aio_write - our write method, implementing support for write(),
 + * writev(), and aio_write(). Writes are our fast path, and we try to 
 optimize
 + * them above all else.
 + */
 +ssize_t logger_aio_write(struct kiocb *iocb, const struct iovec *iov,
 +  unsigned long nr_segs, loff_t ppos)
 +{
 + struct logger_log *log = file_get_log(iocb-ki_filp);
 + size_t orig = log-w_off;
 + struct logger_entry header;
 + struct timespec now;
 + ssize_t ret = 0;
 +
 + now = current_kernel_time();
 +
 + header.pid = current-tgid;
 +

Re: [PATCH] Move an assert under DEBUG_KERNEL.

2011-01-06 Thread Andrew Morton 
On Thu, 6 Jan 2011 02:13:38 -0600
Rob Landley rland...@parallels.com wrote:

 From: Rob Landley rland...@parallels.com
 
 Move an assert under DEBUG_KERNEL.
 
 Signed-off-by: Rob Landley rland...@parallels.com
 ---
 
 Saves about 3k from x86-64 defconfig according to scripts/bloat-o-meter.
 
   include/linux/rtnetlink.h |4 
   1 file changed, 4 insertions(+)
 
 diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
 index bbad657..28c4025 100644
 --- a/include/linux/rtnetlink.h
 +++ b/include/linux/rtnetlink.h
 @@ -782,6 +782,7 @@ extern struct netdev_queue 
 *dev_ingress_queue_create(struct net_device *dev);
   extern void rtnetlink_init(void);
   extern void __rtnl_unlock(void);
 
 +#ifdef CONFIG_DEBUG_KERNEL
   #define ASSERT_RTNL() do { \
   if (unlikely(!rtnl_is_locked())) { \
   printk(KERN_ERR RTNL: assertion failed at %s (%d)\n, \
 @@ -789,6 +790,9 @@ extern void __rtnl_unlock(void);
   dump_stack(); \
   } \
   } while(0)
 +#else
 +#define ASSERT_RTNL()
 +#endif
 
   static inline u32 rtm_get_table(struct rtattr **rta, u8 table)
   {

Probably a worthwhile thing to do, IMO.  If there's some net-specific
CONFIG_DEBUG_ setting then that wold be a better thing to use.

However the patch was a) wordwrapped, b) space-stuffed and c) not cc'ed
to the networking list.  So its prospects are dim.
--
To unsubscribe from this list: send the line unsubscribe linux-embedded in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/3] Decompressors: Add XZ decompressor module

2010-11-24 Thread Andrew Morton 
On Wed, 24 Nov 2010 22:51:52 +0200
Lasse Collin lasse.col...@tukaani.org wrote:

 From: Lasse Collin lasse.col...@tukaani.org
 
 In userspace, the .lzma format has become mostly a legacy
 file format that got superseded by the .xz format. Similarly,
 LZMA Utils was superseded by XZ Utils.
 
 These patches add support for XZ decompression into
 the kernel. Most of the code is as is from XZ Embedded
 http://tukaani.org/xz/embedded.html. It was written for
 the Linux kernel but is usable in other projects too.
 
 Advantages of XZ over the current LZMA code in the kernel:
   - Nice API that can be used by other kernel modules; it's
 not limited to kernel, initramfs, and initrd decompression.
   - Integrity check support (CRC32)
   - BCJ filters improve compression of executable code on
 certain architectures. These together with LZMA2 can
 produce a few percent smaller kernel or Squashfs images
 than plain LZMA without making the decompression slower.
 
 This patch: Add the main decompression code (xz_dec), testing
 module (xz_dec_test), wrapper script (xz_wrap.sh) for the xz
 command line tool, and documentation. The xz_dec module is
 enough to have a usable XZ decompressor e.g. for Squashfs.

I'm not seeing any documentation which tells me how to create, install
and execute xs-compressed kernels. There are new makefile targets?


 ...

 +#define bcj_x86_test_msbyte(b) ((b) == 0x00 || (b) == 0xFF)

This should be written in C.  It looks nicer, and so
bcj_x86_test_msbyte(*p++) won't explode.

 +static noinline_for_stack size_t bcj_x86(

hm, but it uses little stack space.

 +static noinline_for_stack size_t bcj_x86(
 + struct xz_dec_bcj *s, uint8_t *buf, size_t size)

The preferred style is

static noinline_for_stack size_t bcj_x86(struct xz_dec_bcj *s, uint8_t *buf,
size_t size)

or

static noinline_for_stack size_t
bcj_x86(struct xz_dec_bcj *s, uint8_t *buf, size_t size)

(lots of dittoes)
--
To unsubscribe from this list: send the line unsubscribe linux-embedded in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 6/8] lzma: Make lzma available to non initramfs/initrd code

2010-01-06 Thread Andrew Morton 
On Fri, 11 Dec 2009 01:34:08 +
Phillip Lougher phil...@lougher.demon.co.uk wrote:

 Add a config option DECOMPRESS_LZMA_NEEDED which allows subsystems to
 specify they need the unlzma code.  Normally decompress_unlzma.c is
 compiled with __init and unlzma is not exported to modules.
 
 Move INIT definition into separate header files for bzip2/lzma/inflate
 so it can be defined differently for each decompressor.
 

This patch (which is in linux-next) breaks
lib-add-support-for-lzo-compressed-kernels.patch, below.  The
definition of INIT is no longer available in lib/decompress_unlzo.c, and

lib/decompress_unlzo.c: In function 'unlzo':
lib/decompress_unlzo.c:106: error: 'error' undeclared (first use in this 
function)
lib/decompress_unlzo.c:106: error: (Each undeclared identifier is reported only 
once
lib/decompress_unlzo.c:106: error: for each function it appears in.)
lib/decompress_unlzo.c:111: error: implicit declaration of function 'error'


I'm planning on merging 

zlib-optimize-inffast-when-copying-direct-from-output.patch
lib-add-support-for-lzo-compressed-kernels.patch
arm-add-support-for-lzo-compressed-kernels.patch
x86-add-support-for-lzo-compressed-kernels.patch
add-lzo-compression-support-for-initramfs-and-old-style-initrd.patch

into 2.6.33.  I don't immediately remember why I decided that - perhaps
because the patches did arrive in time for .33, but got stalled because
people were screwing around in other trees.

So if I go ahead with that merge, linux-next will need fixing.  And I
didn't get down and work what the appropriate fix is, and I don't want
to break linux-next in serious ways.


So what to do?  I guess I could go ahead with the mainline merge, and
Stephen drops whatever that tree was from linux-next until it has
been fixed up?

If we decide to hold the above five patches off until 2.6.34 then
they'll need to be fixed up to work against linux-next, and they will
be dependent upon your patch series.




From: Albin Tonnerre albin.tonne...@free-electrons.com

This patch series adds generic support for creating and extracting
LZO-compressed kernel images, as well as support for using such images on
the x86 and ARM architectures, and support for creating and using
LZO-compressed initrd and initramfs images.


Russell King said:

: Testing on a Cortex A9 model:
: - lzo decompressor is 65% of the time gzip takes to decompress a kernel
: - lzo kernel is 9% larger than a gzip kernel
: 
: which I'm happy to say confirms your figures when comparing the two.
: 
: However, when comparing your new gzip code to the old gzip code:
: - new is 99% of the size of the old code
: - new takes 42% of the time to decompress than the old code
: 
: What this means is that for a proper comparison, the results get even better:
: - lzo is 7.5% larger than the old gzip'd kernel image
: - lzo takes 28% of the time that the old gzip code took
: 
: So the expense seems definitely worth the effort.  The only reason I
: can think of ever using gzip would be if you needed the additional
: compression (eg, because you have limited flash to store the image.)
: 
: I would argue that the default for ARM should therefore be LZO.


This patch:

The lzo compressor is worse than gzip at compression, but faster at
extraction.  Here are some figures for an ARM board I'm working on:

Uncompressed size: 3.24Mo
gzip  1.61Mo 0.72s
lzo   1.75Mo 0.48s

So for a compression ratio that is still relatively close to gzip, it's
much faster to extract, at least in that case.

This part contains:
 - Makefile routine to support lzo compression
 - Fixes to the existing lzo compressor so that it can be used in
   compressed kernels
 - wrapper around the existing lzo1x_decompress, as it only extracts one
   block at a time, while we need to extract a whole file here
 - config dialog for kernel compression

[a...@linux-foundation.org: coding-style fixes]
[a...@linux-foundation.org: cleanup]
Signed-off-by: Albin Tonnerre albin.tonne...@free-electrons.com
Tested-by: Wu Zhangjin wuzhang...@gmail.com
Acked-by: H. Peter Anvin h...@zytor.com
Cc: Ingo Molnar mi...@elte.hu
Cc: Thomas Gleixner t...@linutronix.de
Tested-by: Russell King r...@arm.linux.org.uk
Acked-by: Russell King r...@arm.linux.org.uk
Cc: Ralf Baechle r...@linux-mips.org
Signed-off-by: Andrew Morton a...@linux-foundation.org
---

 include/linux/decompress/unlzo.h |   10 +
 init/Kconfig |   18 +-
 lib/decompress_unlzo.c   |  209 +
 lib/lzo/lzo1x_decompress.c   |9 -
 scripts/Makefile.lib |5 
 5 files changed, 244 insertions(+), 7 deletions(-)

diff -puN /dev/null include/linux/decompress/unlzo.h
--- /dev/null
+++ a/include/linux/decompress/unlzo.h
@@ -0,0 +1,10 @@
+#ifndef DECOMPRESS_UNLZO_H
+#define DECOMPRESS_UNLZO_H
+
+int unlzo(unsigned char *inbuf, int len,
+   int(*fill)(void*, unsigned int),
+   int(*flush)(void*, unsigned int),
+   unsigned char *output,
+   int *pos,
+   void

Re: [PATCH 1/9] Squashfs: move zlib decompression wrapper code into a separate file

2009-12-07 Thread Andrew Morton 
On Mon, 07 Dec 2009 02:25:08 +
Phillip Lougher phil...@lougher.demon.co.uk wrote:

 +++ b/fs/squashfs/zlib_wrapper.c
 @@ -0,0 +1,109 @@
 +/*
 + * Squashfs - a compressed read only filesystem for Linux
 + *
 + * Copyright (c) 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009
 + * Phillip Lougher phil...@lougher.demon.co.uk
 + *
 + * This program is free software; you can redistribute it and/or
 + * modify it under the terms of the GNU General Public License
 + * as published by the Free Software Foundation; either version 2,
 + * or (at your option) any later version.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License
 + * along with this program; if not, write to the Free Software
 + * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
 + *
 + * zlib_wrapper.c
 + */
 +
 +
 +#include linux/mutex.h
 +#include linux/buffer_head.h
 +#include linux/zlib.h
 +
 +#include squashfs_fs.h
 +#include squashfs_fs_sb.h
 +#include squashfs_fs_i.h
 +#include squashfs.h
 +
 +int zlib_uncompress(struct squashfs_sb_info *msblk, void **buffer,
 + struct buffer_head **bh, int b, int offset, int length, int srclength,
 + int pages)

This isn't a very good function name.  zlib_uncompress() now becomes a
kernel-wide identifier, but it's part of squashfs, not part of zlib.

Maybe that gets fixed in a later patch.  If so, thwap me.
--
To unsubscribe from this list: send the line unsubscribe linux-embedded in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: New MMC maintainer needed

2009-08-12 Thread Andrew Morton 

davem has set up linux-...@vger.kernel.org for us.  It will be archived
at marc.info.  Thanks to both..

I was thinking that we wouldn't need a separate list but then I
realised I didn't have anywhere to send MMC bug reports.

echo subscribe linux-mmc | mail majord...@vger.kernel.org


--
To unsubscribe from this list: send the line unsubscribe linux-embedded in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] lib/decompress_*: only include linux/slab.h if STATIC is not defined

2009-08-04 Thread Andrew Morton 
On Mon,  3 Aug 2009 16:58:16 +0200
Albin Tonnerre albin.tonne...@free-electrons.com wrote:

 These includes were added by 079effb6933f34b9b1b67b08bd4fd7fb672d16ef to
 fix the build when using kmemtrace. However this is not necessary when
 used to create a compressed kernel, and actually creates issues (brings
 a lot of things unavailable in the decompression environment), so don't
 include it if STATIC is defined.
 

The description actually creates issues (brings a lot of things
unavailable in the decompression environment) is inadequate.  Please
describe te problem this patch fixes more completely so that others
(ie: me) can decide whether this patch is needed in 2.6.32, 2.6.31. 
2.6.30, ...


This patch conflicts heavily with

http://userweb.kernel.org/~akpm/mmotm/broken-out/bzip2-lzma-remove-nasty-uncompressed-size-hack-in-pre-boot-environment.patch


What should we do about that?


--
To unsubscribe from this list: send the line unsubscribe linux-embedded in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/6] include/linux/unaligned/{l,b}e_byteshift.h: Fix usage for compressed kernels

2009-08-04 Thread Andrew Morton 
On Mon,  3 Aug 2009 16:58:17 +0200
Albin Tonnerre albin.tonne...@free-electrons.com wrote:

 When unaligned accesses are required for uncompressing a kernel (such as
 for LZO decompression on ARM in a patch that follows), including
 linux/kernel.h causes issues as it brings in a lot of things that are
 not available in the decompression environment.
 However, those files apparently use nothing from linux/kernel.h, all
 they need is the declaration of types such as u32 or u64, so
 linux/types.h should be enough

Again, please provide a full description of thes issues which a patch
addresses so that the patch's importance can be understood by others,
thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-embedded in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Add support for LZO-compressed kernels

2009-08-04 Thread Andrew Morton 
On Mon,  3 Aug 2009 16:58:18 +0200
Albin Tonnerre albin.tonne...@free-electrons.com wrote:

 This is the first part of the lzo patch

Please pass this patch (and all others!) through scritps/checkpatch.pl.

checkpatch reports a number of trivial errors which you wouldn't have included
had you known about them.
--
To unsubscribe from this list: send the line unsubscribe linux-embedded in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: New MMC maintainer needed

2009-08-03 Thread Andrew Morton 
On Mon, 03 Aug 2009 14:13:28 +0300
Adrian Hunter adrian.hun...@nokia.com wrote:

 Pierre Ossman wrote:
  On Fri, 31 Jul 2009 11:54:07 +0100
  Matt Fleming m...@console-pimps.org wrote:
  
  On Fri, Jul 31, 2009 at 12:26:23PM +0200, Pierre Ossman wrote:
  [PATCH 0/32] mmc and omap_hsmmc patches
  http://marc.info/?t=124722953900010r=1w=2
 
  I haven't looked through these at all. The ones affecting the core
  probably need some thorough reviews.
 
  I did notice the patch to say which cards a controller supports though,
  and I'm very sceptical about that one. The scanning process should work
  anyway, and the performance impact should be negligible as it is only
  on init. So that patch only adds complexity and confusion IMO.
 
  How much complexity does it really add? Surely it's better to give the
  host controller driver writers the ability to not entertain supporting
  some cards if they cannot be used? If they want to avoid the scanning
  process for certain cards, why not let them?
 
  
  Let's look at the pros and cons of this:
 
 But the cons are all subjective.
 
  Con:
  
   - The scanning code gets less clear as you increase the number of
 possible paths through it.
  
   - Different systems will have different init sequences, possibly
 provoking bugs in the cards.
  
   - Host driver writers now have more capability bits they have to
 consider. And these might be less than obvious since SD/MMC/SDIO are
 normally compatible so these bits seem useless.
  
   - With the current logic (which was better in the first version),
 normal drivers will have to explicitly state that they work as
 intended by setting all bits.

Subjective or not, they are real and valid.

 And the pro is objective.
 
  Pro:
  
   - A slightly reduced scanning time.
 
 That's great!  Why do you disregard this so easily?

Coz nobody quantified it.  Let's go to the changelog:

: Some hosts can accept only certain types of cards.  For example, an eMMC
: is MMC only and a uSD slot may be SD only.  However the MMC card scanning
: logic checks for all card types one by one.
: 
: Add host capabilities to specify which card types can be used, and amend
: the card scanning logic to skip scanning for those types which cannot be
: used.

See, it provides no reason at all for adding this additional complexity.

Now we're told it's faster.  OK.  How much faster?  Show us the
numbers so we can all understand why the change is justifiable!

--
To unsubscribe from this list: send the line unsubscribe linux-embedded in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: New MMC maintainer needed

2009-07-24 Thread Andrew Morton 
On Thu, 23 Jul 2009 14:52:09 +0100
Matt Fleming m...@console-pimps.org wrote:

 On Thu, Jul 23, 2009 at 09:50:03AM +0300, Ohad Ben-Cohen wrote:
  Hi Andrew,
  
  On Thu, Jul 23, 2009 at 9:22 AM, Andrew Mortona...@linux-foundation.org 
  wrote:
   I actually already have a little pile of MMC things queued:
  
  Please also consider queuing up the attached patch as well.
  
  The patch is removing the current SDIO cards 1.8V limit, which is
  needed for embedded
  SDIO devices like TI 127x WLAN devices (with 1.8V MMC controllers like we
  have on the ZOOM2 boards for example).
  
  Thank you,
  Ohad.
 
  From f9ba45b537dd12fc09443ee29c48860665f8ac82 Mon Sep 17 00:00:00 2001
  From: Ohad Ben-Cohen o...@bencohen.org
  Date: Wed, 15 Jul 2009 09:21:41 +0300
  Subject: [PATCH] sdio: do not ignore MMC_VDD_165_195
  
  This is needed for 1.8V embedded SDIO devices and supporting host 
  controllers
  (e.g. TI 127x and ZOOM2 boards)
  
  Signed-off-by: Ohad Ben-Cohen o...@bencohen.org
  ---
   drivers/mmc/core/sdio.c |7 ---
   1 files changed, 0 insertions(+), 7 deletions(-)
  
  diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
  index fb99ccf..6f221dc 100644
  --- a/drivers/mmc/core/sdio.c
  +++ b/drivers/mmc/core/sdio.c
  @@ -275,13 +275,6 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
  ocr = ~0x7F;
  }
   
  -   if (ocr  MMC_VDD_165_195) {
  -   printk(KERN_WARNING %s: SDIO card claims to support the 
  -  incompletely defined 'low voltage range'. This 
  -  will be ignored.\n, mmc_hostname(host));
  -   ocr = ~MMC_VDD_165_195;
  -   }
  -
  host-ocr = mmc_select_voltage(host, ocr);
   
  /*
  -- 
  1.5.4.3
  
 
 Looks OK to me, I'm unaware of a reason to not allow a card to use
 MMC_VDD_165_195 if that's what it wants.
 

Yes, that code was there from day #1, via Pierre's original commit.  There
is no indication in either the code comments or the changelog why it was done
this way.

Other ways of finding out why this code is there include:

a) search the mailing list for review discussion.  I can't find it
   in my lkml archive.

b) ask Pierre :)
--
To unsubscribe from this list: send the line unsubscribe linux-embedded in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: New MMC maintainer needed

2009-07-23 Thread Andrew Morton 
On Thu, 23 Jul 2009 06:54:47 +0100 Matt Fleming m...@console-pimps.org wrote:

 On Thu, Jul 23, 2009 at 01:08:08AM +0100, Ian Molton wrote:
  Andrew Morton wrote:
 
  Until and unless someone else steps up I can act as maintainer of last
  resort for MMC.  As I'm presently doing for fbdev, hwmon, rtc, spi,
  gpio, i2o and about 1000 other identifiable subsystems.
 
  If no-one else is helping out, Im happy to also be CC'd on MMC patches.
 
 
 Ditto.

Thanks, guys.

I actually already have a little pile of MMC things queued:

http://userweb.kernel.org/~akpm/mmotm/broken-out/mmc-in-mmc_power_up-use-previously-selected-ocr-if-available.patch
http://userweb.kernel.org/~akpm/mmotm/broken-out/omap-hsmmc-do-not-enable-buffer-ready-interrupt-if-using-dma.patch
http://userweb.kernel.org/~akpm/mmotm/broken-out/mmc-msm_sdccc-driver-for-htc-dream.patch
http://userweb.kernel.org/~akpm/mmotm/broken-out/msm_sdccc-convert-printkkern_level-to-pr_level.patch
http://userweb.kernel.org/~akpm/mmotm/broken-out/msm_sdccc-stylistic-cleaning.patch
http://userweb.kernel.org/~akpm/mmotm/broken-out/msm_sdccc-move-overly-indented-code-to-separate-function.patch

I guess we own them now ;)
--
To unsubscribe from this list: send the line unsubscribe linux-embedded in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: New MMC maintainer needed

2009-07-23 Thread Andrew Morton 
On Thu, 23 Jul 2009 17:25:59 +1000 Stephen Rothwell s...@canb.auug.org.au 
wrote:

 Hi Andrew,
 
 On Wed, 22 Jul 2009 23:22:59 -0700 Andrew Morton a...@linux-foundation.org 
 wrote:
 
  I actually already have a little pile of MMC things queued:
  
  http://userweb.kernel.org/~akpm/mmotm/broken-out/mmc-in-mmc_power_up-use-previously-selected-ocr-if-available.patch
  http://userweb.kernel.org/~akpm/mmotm/broken-out/omap-hsmmc-do-not-enable-buffer-ready-interrupt-if-using-dma.patch
  http://userweb.kernel.org/~akpm/mmotm/broken-out/mmc-msm_sdccc-driver-for-htc-dream.patch
  http://userweb.kernel.org/~akpm/mmotm/broken-out/msm_sdccc-convert-printkkern_level-to-pr_level.patch
  http://userweb.kernel.org/~akpm/mmotm/broken-out/msm_sdccc-stylistic-cleaning.patch
  http://userweb.kernel.org/~akpm/mmotm/broken-out/msm_sdccc-move-overly-indented-code-to-separate-function.patch
  
  I guess we own them now ;)
 
 So how about, as an experiment, you put all those (at least those that are
 stable) into a git tree (or quilt series) based on Linus' tree and I
 could replace the current linux-next mmc tree (which is empty) with that.
 

I haven't worked out how to do that yet :( (Well, I have, but it's all
complex).

It gets easy if I remove linux-next.patch from -mm.  Maybe I'll do that.
--
To unsubscribe from this list: send the line unsubscribe linux-embedded in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: New MMC maintainer needed

2009-07-22 Thread Andrew Morton 
On Tue, 14 Jul 2009 15:36:01 +0200
Pierre Ossman pie...@ossman.eu wrote:

 I'm afraid the time has come for me to step down as maintainer for the
 MMC/SD/SDIO subsystem and for someone else to take over the rudder. It
 is no secret that I haven't been able to give the maintainer role the
 resources it required, and as a result it has gone from being a fun
 hobby to a burdensome chore. I was hoping it was a temporary slump, but
 this has been the norm for over a year now, so it's time for me to
 throw in the towel.
 
 I don't have any clear nominees for a replacement, but if you feel that
 you are in a position to take over the maintainer role then please
 raise your hand.
 
 I only have a few patches in my queue right now and I can push those
 off to Linus before I quit. There are however a lot of unhandled
 messages that need attention. I can provide a list once a successor has
 been named.
 
 I realise that you have put a lot of time and effort into the patches
 you send me and I am truly sorry that I haven't been able to handle
 them better. Hopefully me stepping down means that we get someone who
 can actually take care of everything in a timely manner.
 
 I haven't decided what to do with the sdhci maintainership yet. Right
 now I only know I need a break and freeing myself from the MMC
 maintainer role is the first step. Hopefully I can return as a
 developer in the future.
 

Thanks, Pierre.

Until and unless someone else steps up I can act as maintainer of last
resort for MMC.  As I'm presently doing for fbdev, hwmon, rtc, spi,
gpio, i2o and about 1000 other identifiable subsystems.

Fortunately, MMC doesn't appear to have its own mailing list so I'll
actually get to see the patches.  I do monitor a couple of
subsystem-specific lists (fbdev, hwmon) but the delays can be awful - a
month or two.


When it comes to subsystems about which I have little knowledge I tend
to work as follows:

- scan the patch to see if anything stands out to my inexperienced eye

- if possible, comment on it to make a bit of noise and to let the
  submitter and others know that a) someone has looked at it and b)
  there's a chance that it'll be merged.

- try to identify some people who might have an interest in the patch
  and/or who might have related experience which would help them review
  this one effectively.

So if someone finds themselves cc'ed on an MMC email from myself,
please do take a bit of time to pass an eye across it, and let us know
that you have done so - it really helps.

--
To unsubscribe from this list: send the line unsubscribe linux-embedded in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Ksummit-2009-discuss] Representing Embedded Architectures at the Kernel Summit

2009-06-03 Thread Andrew Morton 
On Wed, 3 Jun 2009 18:09:25 +0100
Russell King r...@arm.linux.org.uk wrote:

 In
 fact, on ARM the DMA mask is exactly that - it's a 100% proper mask.  It's
 not a bunch of zeros in the MSB followed by a bunch of ones down to the
 LSB.  It can be a bunch of ones, a bunch of zeros, followed by a bunch of
 ones.
 
 The way we occasionally have to deal with this is to trial an allocation,
 see if the physical address fits, if not free the page and try again with
 GFP_DMA set.

A couple of times I've suggested that we have the ability to allocate
one zone per address bit, so a 32-bit machine with 4k pages would end
up having 20 zones.  Then, your funny DMA mask can be directly passed
into the page allocator as a zone mask and voila, I think.

 There's many stories I've heard on what is supposed to take care of the
 coherency that I now just close my ears to the problem and chant it
 doesn't exist, people aren't seeing it, mainline folk just don't give
 a damn.  Really.  It is a problem on _some_ ARM devices and has been
 for several years now, and I've 100% given up caring about it.

I wasn't even aware that there was an issue here.  Please don't blame
mainline folk for something they weren't told about!

--
To unsubscribe from this list: send the line unsubscribe linux-embedded in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch] Wait for console to become available, ver 3

2009-04-20 Thread Andrew Morton 
On Fri, 17 Apr 2009 14:31:48 -0700
David VomLehn dvoml...@cisco.com wrote:

 Parallelization to improve boot times has been successful enough that race
 conditions now exist between the init_post() open of /dev/console and
 initialization of the console device. When this occurs, opening /dev/console
 fails and any applications inherited from init have no standard in/out/error
 devices. This is expected behavior if no console device is available, but
 quite unfortunate in the case where the console is just a bit slow waking up.
 
 Some buses, such as USB, offer no guarantees about how long it takes to
 discover devices, so there is no reliable way to distinguish between a missing
 console and a slow one.  The pragmatic approach taken in this patch is to
 wait for a while to see if a console shows up, and just go on if it doesn't.
 The default delay is 1000 msec (1 second).
 
 There are two new command line parameters:
 consolewait   Wait forever for a console to be registered
 consoledelay=msec Use the given number of milliseconds as the delay
   interval instead of the default

This is all pretty nasty, isn't it?

Let's step back for a bit from any implementation and ask what is the
ideal behaviour?  I think it's one of

a) we permit init_post()'s open() to succeed.  Console output is
   buffered by the kernel (could be in the printk log_buf).  When the
   initial console is eventually registered, we flush all the queued
   characters into it.

b) we block in init_post(), waiting for the initial console to
   become available.

I think b) is better.  Simpler, safer, less likely for information loss
if the kernel were to crash in that delay window.

Am I right?  Did I miss any options?

If we want b) then how to do it?

One possibility: the initcalls have been completed when init_post() is
called.  How about: if one of those initcalls will be asynchronously
registering a console, it should inform the console layer about this. 
It should call the new i_will_be_adding_a_console_soon() function
within its initcall.  The console subsystem will remember this, and we
can cause init_post() to block until that registration has occurred.

We'll need to be able to handle errors, and we'll need to be able to
handle the case where the initcall function isn't sure whether or not
it will be registering a console.  So there will also need to be an
oops_im_not_adding_a_console_after_all() function, which will withdraw
the effects of i_will_be_adding_a_console_soon().

Which means that i_will_be_adding_a_console_soon() will need to return
a handle for the oops_im_not_adding_a_console_after_all() caller to
pass.

If init_post() is currently blocked awaiting the arrival of the
console, oops_im_not_adding_a_console_after_all() will unblock
init_post() if there are no more potential console registrations
pending, and inti_post()'s attempt to open a console will fail.


Or something like that?
--
To unsubscribe from this list: send the line unsubscribe linux-embedded in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch] Wait for console to become available, ver 3

2009-04-20 Thread Andrew Morton 
On Mon, 20 Apr 2009 17:51:16 -0400 (EDT)
Alan Stern st...@rowland.harvard.edu wrote:

 On Mon, 20 Apr 2009, Andrew Morton wrote:
 
  If we want b) then how to do it?
  
  One possibility: the initcalls have been completed when init_post() is
  called.  How about: if one of those initcalls will be asynchronously
  registering a console, it should inform the console layer about this. 
  It should call the new i_will_be_adding_a_console_soon() function
  within its initcall.  The console subsystem will remember this, and we
  can cause init_post() to block until that registration has occurred.
  
  We'll need to be able to handle errors, and we'll need to be able to
  handle the case where the initcall function isn't sure whether or not
  it will be registering a console.  So there will also need to be an
  oops_im_not_adding_a_console_after_all() function, which will withdraw
  the effects of i_will_be_adding_a_console_soon().
  
  Which means that i_will_be_adding_a_console_soon() will need to return
  a handle for the oops_im_not_adding_a_console_after_all() caller to
  pass.
  
  If init_post() is currently blocked awaiting the arrival of the
  console, oops_im_not_adding_a_console_after_all() will unblock
  init_post() if there are no more potential console registrations
  pending, and inti_post()'s attempt to open a console will fail.
  
  
  Or something like that?
 
 What if a subsystem simply doesn't know in advance whether or not it's 
 going to register a console?  Or doesn't know when it has finished 
 probing all devices (since a new device could be plugged in at any 
 time)?

Fix it.  It's trivial to make a sub-driver call back into a higher
layer to tell it that it registered a console.  Or just do the
i_will_be_adding_a_console_soon()/oops_im_not_adding_a_console_after_all()
calls from the layer which _does_ know.

 Not to mention that this scheme appears more complicated than the one 
 it's intended to replace... although it doesn't have any boot-line 
 parameters.

It isn't very complicated.

Yes, a boot parameter is simple to inplement.  But it's ghastly from
a usability POV.  Especially if you care about boot times.  For how
long do you delay?  The user has to experiment with different delays
until he finds the magic number.  Then he adds 10% and waits for the
inevitable failure reports to come in.

It's much better to just get it right, even if that makes it more
complex.


--
To unsubscribe from this list: send the line unsubscribe linux-embedded in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch] Wait for console to become available, ver 3

2009-04-20 Thread Andrew Morton 
On Mon, 20 Apr 2009 15:35:00 -0700
David VomLehn dvoml...@cisco.com wrote:

 On Mon, Apr 20, 2009 at 03:14:00PM -0700, Andrew Morton wrote:
  On Mon, 20 Apr 2009 17:51:16 -0400 (EDT)
  Alan Stern st...@rowland.harvard.edu wrote:
 ...
   What if a subsystem simply doesn't know in advance whether or not it's 
   going to register a console?  Or doesn't know when it has finished 
   probing all devices (since a new device could be plugged in at any 
   time)?
  
  Fix it.  It's trivial to make a sub-driver call back into a higher
  layer to tell it that it registered a console.  Or just do the
  i_will_be_adding_a_console_soon()/oops_im_not_adding_a_console_after_all()
  calls from the layer which _does_ know.
 
 In the case of the console, we already have register_console(), which is
 what I'm using. I think your proposal will require adding code all over
 the place. And buses such as USB simply have no way of knowing whether they
 are done enumerating devices. A new device could take hours to come on line.

Add a timeout parameter to i_will_be_adding_a_console_soon().  (This
means that the how-long-to-wait-for policy is probably ahrd-coded into
the kernel which might be a problem).

  Yes, a boot parameter is simple to inplement.  But it's ghastly from
  a usability POV.  Especially if you care about boot times.  For how
  long do you delay?  The user has to experiment with different delays
  until he finds the magic number.  Then he adds 10% and waits for the
  inevitable failure reports to come in.
  
  It's much better to just get it right, even if that makes it more
  complex.
 
 With USB, you just can't *ever* get it right. There is no limit on how
 long a device has to tell you its there. I wish this weren't the case,
 but our good friends in the USB world tell us that we have been lucky
 to have had USB consoles work as long as they have.

Sigh, OK, I appreciate the problem better.  But the proposed solution
is really quite fragile.  I expect that it will only prove usable in
highly controlled hardware setups.

Is my option a) any use?
--
To unsubscribe from this list: send the line unsubscribe linux-embedded in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 RESEND] gpiolib: Add pin change notification

2008-10-21 Thread Andrew Morton 
On Tue, 21 Oct 2008 09:50:06 +1100
Ben Nizette [EMAIL PROTECTED] wrote:

 
 This adds pin change notification to the gpiolib sysfs interface. It
 requires 16 extra bytes in gpio_desc iff CONFIG_GPIO_SYSFS which in turn
 means, eg, 4k of .bss usage on AVR32.  Due to limitations in sysfs, this
 patch makes poll(2) and friends work as expected on the value
 attribute, though reads on value will never block and there is no
 facility for async reads and writes.
 

 ...

 +struct poll_desc *work_to_poll(struct work_struct *ws)

static

 +{
 + return container_of((struct delayed_work *)ws, struct poll_desc, work);
 +}
 +
 +void gpio_poll_work(struct work_struct *ws)

static

 +{
 + struct poll_desc*poll = work_to_poll(ws);
 +
 + unsignedgpio = poll-gpio;
 + struct gpio_desc*desc = gpio_desc[gpio];
 +
 + int new = gpio_get_value_cansleep(gpio);
 + int old = desc-val;
 +
 + if ((new  !old  test_bit(ASYNC_RISING, desc-flags)) ||
 + (!new  old  test_bit(ASYNC_FALLING, desc-flags)))
 + sysfs_notify(desc-dev-kobj, NULL, value);
 +
 + desc-val = new;
 + schedule_delayed_work(poll-work, desc-timeout);
 +}
 +
 +static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
 +{
 + struct gpio_desc *desc = dev_id;
 + int gpio = desc - gpio_desc;
 + int new, old;
 +
 + if (!gpio_is_valid(gpio))
 + return IRQ_NONE;
 +
 + new = gpio_get_value(gpio);
 + old = desc-val;
 +
 + if ((new  !old  test_bit(ASYNC_RISING, desc-flags)) ||
 + (!new  old  test_bit(ASYNC_FALLING, desc-flags)))
 + sysfs_notify(desc-dev-kobj, NULL, value);

eekeekeek!  sysfs_notify() does mutex_lock() and will die horridly if
called from an interrupt handler.

You should have got a storm of warnings when runtime testing this code.
Please ensure that all debug options are enabled when testing code. 
Documentation/SubmitChecklist has help.

 + desc-val = new;
 +
 + return IRQ_HANDLED;
 +}
 +
  static ssize_t gpio_direction_show(struct device *dev,
   struct device_attribute *attr, char *buf)
  {
 @@ -284,9 +351,162 @@ static ssize_t gpio_value_store(struct d
  static /*const*/ DEVICE_ATTR(value, 0644,
   gpio_value_show, gpio_value_store);
  
 +static ssize_t gpio_notify_show(struct device *dev,
 + struct device_attribute *attr, char *buf)
 +{
 + const struct gpio_desc  *desc = dev_get_drvdata(dev);
 + ssize_t ret;
 +
 + mutex_lock(sysfs_lock);
 +
 + if (test_bit(ASYNC_MODE_IRQ, desc-flags))
 + ret = sprintf(buf, irq\n);
 + else if (test_bit(ASYNC_MODE_POLL, desc-flags))
 + ret = sprintf(buf, %ld\n, desc-timeout * 1000 / HZ);
 + else
 + ret = sprintf(buf, none\n);
 +
 + mutex_unlock(sysfs_lock);
 + return ret;
 +}

panics

oh, sysfs_lock is a gpiolib-local thing, not a sysfs-core thing. 
Crappy name.


--
To unsubscribe from this list: send the line unsubscribe linux-embedded in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 21/23] make section names compatible with -ffunction-sections -fdata-sections: v850

2008-07-02 Thread Andrew Morton 
On Wed, 2 Jul 2008 23:45:38 +0200
Arnd Bergmann [EMAIL PROTECTED] wrote:

 On Wednesday 02 July 2008, Denys Vlasenko wrote:
  This patch fixes v850 architecture.
 
 For all I know, v850 has been broken and unmaintained for a few years now,
 didn't someone have a patch to remove it entirely?
 

yup, it's queued for 2.6.27-rc1.
--
To unsubscribe from this list: send the line unsubscribe linux-embedded in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html