* Re: introduce dm-snap-mv
From: Daniel Phillips @ 2010-10-07 21:31 UTC (permalink / raw)
To: Cong Meng
Cc: linux-fsdevel, linux-kernel, dm-devel, Andrew Morton,
Alexander Viro, Christoph Hellwig, Nick Piggin
In-Reply-To: <20101006083150.GA15758@zhongling>
Hi Meng,
The patch looks sensible, however the question is: why do you want to
do this? Would it not be better to generalize your metadata format to
accomodate the device's native blocksize?
Regards,
Daniel
> a kernel patch
> --------------
> Now, dm-snap-mv highly depends on a kernel patch below, which make __getblk()
> can get a 4K buffer head while block size of the disk is NOT 4K.
>
> Signed-off-by: Cong Meng <mcpacino@gmail.com>
> ---
> fs/buffer.c | 7 ++-----
> 1 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 3e7dca2..f7f9d33 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -1051,10 +1051,7 @@ grow_buffers(struct block_device *bdev, sector_t block, int size)
> pgoff_t index;
> int sizebits;
>
> - sizebits = -1;
> - do {
> - sizebits++;
> - } while ((size << sizebits) < PAGE_SIZE);
> + sizebits = PAGE_CACHE_SHIFT - bdev->bd_inode->i_blkbits;
>
> index = block >> sizebits;
>
> @@ -2924,7 +2921,7 @@ int submit_bh(int rw, struct buffer_head * bh)
> */
> bio = bio_alloc(GFP_NOIO, 1);
>
> - bio->bi_sector = bh->b_blocknr * (bh->b_size >> 9);
> + bio->bi_sector = bh->b_blocknr << (bh->b_bdev->bd_inode->i_blkbits - 9);
> bio->bi_bdev = bh->b_bdev;
> bio->bi_io_vec[0].bv_page = bh->b_page;
> bio->bi_io_vec[0].bv_len = bh->b_size;
^ permalink raw reply
* RE: pvops Dom0 graphics doesnt work with Intel i915
From: Kay, Allen M @ 2010-10-07 22:05 UTC (permalink / raw)
To: sanjay kushwaha, Konrad Rzeszutek Wilk; +Cc: Jeremy Fitzhardinge, xen-devel
In-Reply-To: <AANLkTi=ofd6F4hN2pp=vmmJu5vfp6kDkVs4MBJw9PLvi@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 11403 bytes --]
Sanjay,
Have you tried the latest xen-unstable staging tree? It has a workaround for T410 VT-d graphics issue caused by Lenovo BIOS. This might be the workaround you need:
http://xenbits.xensource.com/staging/xen-unstable.hg?rev/4beee5779122
Allen
From: xen-devel-bounces@lists.xensource.com [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of sanjay kushwaha
Sent: Thursday, October 07, 2010 12:13 PM
To: Konrad Rzeszutek Wilk
Cc: Jeremy Fitzhardinge; xen-devel
Subject: Re: [Xen-devel] pvops Dom0 graphics doesnt work with Intel i915
Hi Konrad,
Unfortunately T410 doesnt have a serial port and neither does the docking station. I havent had any success with USB-to-Serial port dongles in the past. is there any way to get the serial port output?
Thanks,
Sanjay
On Thu, Oct 7, 2010 at 11:05 AM, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com<mailto:konrad.wilk@oracle.com>> wrote:
On Thu, Oct 07, 2010 at 10:36:34AM -0700, sanjay kushwaha wrote:
> Hi Konrad,
> I tried your tree. It created a 2.6.32.15 based pvops kernel but graphics
> with VT-d still doesn't work. when I give iommu=0 on xen kernel command line
> in grub menu, graphics works but with iommu=1 it doesnt work (The whole
> screen is garbage).
Are there any warnings/debug messages in the serial log? Please follow
the PVOPS Wiki on how to enable all the debug options.
>
>
> On Wed, Oct 6, 2010 at 6:07 PM, Konrad Rzeszutek Wilk <
> konrad.wilk@oracle.com<mailto:konrad.wilk@oracle.com>> wrote:
>
> > On Wed, Oct 06, 2010 at 04:02:51PM -0700, sanjay kushwaha wrote:
> > > Thanks Pasi.
> > >
> > > Hi Konrad,
> > > Could you please let me know how to get these backported drivers as
> > > indicated by Pasi? This is the tree that I have.
> >
> > Just follow the Wiki. Oh, I need to update it.
> >
> > Here do this:
> >
> > git remote add konrad git://
> > git.kernel.org/pub/scm/linux/kernel/git/konrad/xen.git<http://git.kernel.org/pub/scm/linux/kernel/git/konrad/xen.git>
> >
> > git pull konrad
> > git checkout konrad/devel/next.drm
> >
> > make
> >
> > >
> > > [evans@vwifi0 linux-2.6.32.x]$ git show
> > > commit b297cdac0373625d3cd0e6f2b393570dcf2edba6
> > > Merge: c6cfd01 64392f6
> > > Author: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com<mailto:jeremy.fitzhardinge@citrix.com>>
> > > Date: Mon Sep 13 14:27:24 2010 -0700
> > >
> > > Merge branch 'xen/next' into xen/next-2.6.32
> > >
> > > * xen/next:
> > > xen/netfront: Fix another potential race condition
> > > Revert "xen/netfront: default smartpoll to on"
> > >
> > > [evans@vwifi0 linux-2.6.32.x]$
> > >
> > >
> > > Thanks,
> > > Sanjay
> > >
> > > On Wed, Oct 6, 2010 at 1:12 PM, Pasi Kärkkäinen <pasik@iki.fi<mailto:pasik@iki.fi>> wrote:
> > >
> > > > On Wed, Oct 06, 2010 at 10:50:57AM -0700, sanjay kushwaha wrote:
> > > > > Hi,
> > > > > I have run into more problems now. This time with VT-d.
> > > > > When I enable VT-d on this laptop, graphics again stops working in
> > > > dom0
> > > > > with pvops (linux 2.6.32.21). the screen starts showing garbage as
> > > > soon as
> > > > > it switches into graphics mode.this happens when I boot the pvops
> > > > kernel
> > > > > both as dom0 and native linux. However, when I try 2.6.33 based
> > pvops
> > > > > kernel (stable-2.6.33.x) graphics seems to work fine with VT-d
> > when
> > > > > running native but it doesnt work when running as Dom0.
> > > > >
> > > > > so now the problem is:
> > > > >
> > > > > with stable-2.6.32.x: graphics works in Dom0 without Vt-d but not
> > with
> > > > > VT-d (neither native nor Dom0).
> > > > > with stable-2.6.33.x: graphics works with VT-d when running native
> > but
> > > > > doesnt work when running as Dom0 (with or without VT-d).
> > > > >
> > > >
> > > > stable-2.6.33.x is not maintained, and you shouldn't use it.
> > > >
> > > > I think Konrad has a backport of the 2.6.34 drm/dri drivers
> > > > to stable-2.6.32.x somewhere.. that might help.
> > > >
> > > > http://wiki.xensource.com/xenwiki/XenPVOPSDRM
> > > >
> > > > -- Pasi
> > > >
> > > > > I am experiencing this problem both with Lenovo T410, and Dell
> > > > latitude
> > > > > E6410.
> > > > > Has anybody experienced this problem?
> > > > >
> > > > > Thanks,
> > > > > Sanjay
> > > > >
> > > > > On Fri, Oct 1, 2010 at 11:06 AM, sanjay kushwaha
> > > > > <[1]sanjay.kushwaha@gmail.com<mailto:sanjay.kushwaha@gmail.com>> wrote:
> > > > >
> > > > > havent tried stable-2.6.32.x on Radeon. It works with nomodeset
> > and
> > > > > nopat options with stable-2.6.33.x branch.
> > > > >
> > > > > On Fri, Oct 1, 2010 at 10:57 AM, Konrad Rzeszutek Wilk
> > > > > <[2]konrad.wilk@oracle.com<mailto:konrad.wilk@oracle.com>> wrote:
> > > > >
> > > > > On Fri, Oct 01, 2010 at 10:06:48AM -0700, sanjay kushwaha
> > wrote:
> > > > > > When I dont use nomodeset option, dom0 boots fine X runs
> > > > properly.
> > > > > So Fedora
> > > > > > 13 (X86_64) distro with stable-2.6.32.x pvops kernel and
> > > > > xen-unstable works
> > > > > > fine for i915 without nomodeset option.
> > > > >
> > > > > Good to hear it works for you.
> > > > >
> > > > > What about your radeon laptop?
> > > > > >
> > > > > > Thanks,
> > > > > > Sanjay
> > > > > >
> > > > > > On Wed, Sep 29, 2010 at 3:56 PM, sanjay kushwaha
> > > > > > <[3]sanjay.kushwaha@gmail.com<mailto:sanjay.kushwaha@gmail.com>>wrote:
> > > > > >
> > > > > > > Hi Jeremy,
> > > > > > > I switched to stable-2.6.32.x branch (which is 2.6.32.21
> > > > based)
> > > > > but I get
> > > > > > > the same problem. Attached is the Xorg.0.log file when I
> > > > booted
> > > > > with
> > > > > > > nomodeset option.
> > > > > > >
> > > > > > > interestingly I did not see any kernel or driver crash
> > > > messages in
> > > > > the
> > > > > > > dmesg output. I do see these messages multiple times in
> > > > > /var/log/messages
> > > > > > > *
> > > > > > > Sep 29 15:40:32 vwifi0 gdm-binary[2244]: WARNING:
> > GdmDisplay:
> > > > > display
> > > > > > > lasted 0.048984 seconds
> > > > > > > Sep 29 15:40:32 vwifi0 gdm-binary[2244]: WARNING:
> > > > > GdmLocalDisplayFactory:
> > > > > > > maximum number of X display failures reached: check X
> > server
> > > > log
> > > > > for errors
> > > > > > > *
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Sanjay
> > > > > > >
> > > > > > >
> > > > > > > On Wed, Sep 29, 2010 at 11:57 AM, Jeremy Fitzhardinge
> > > > > <[4]jeremy@goop.org<mailto:jeremy@goop.org>>wrote:
> > > > > > >
> > > > > > >> On 09/29/2010 11:12 AM, sanjay kushwaha wrote:
> > > > > > >> > Hi Folks,
> > > > > > >> > I am trying to boot latest xen-unstable on my laptop
> > which
> > > > has
> > > > > Intel
> > > > > > >> > i915 graphics. PVOPS dom0 is 2.6.33.6 based (from
> > branch
> > > > > > >> > xen/stable-2.6.33.x)
> > > > > > >>
> > > > > > >> Don't use that branch; it isn't supported (in fact, I
> > deleted
> > > > it
> > > > > a while
> > > > > > >> ago). Use xen/stable-2.6.32.x for now.
> > > > > > >>
> > > > > > >> J
> > > > > > >>
> > > > > > >> > and the distro is fedora 13 64-bit. The graphics doesnt
> > > > come up
> > > > > and it
> > > > > > >> > seems that i915 driver is crashing multiple times. If I
> > > > boot in
> > > > > > >> > run-level 3 (without X) dom0 boots fine.
> > > > > > >> > I tried booting the dom0 kernel with nomodeset and
> > nopat
> > > > > options
> > > > > > >> > without any success. I searched on internet and found
> > that
> > > > > multiple
> > > > > > >> > people have reported similar problem but I could not
> > find
> > > > any
> > > > > solution.
> > > > > > >> >
> > > > > > >> > Has anybody found a solution or workaround to this
> > problem?
> > > > > > >> >
> > > > > > >> > Thanks,
> > > > > > >> > Sanjay
> > > > > > >> >
> > > > > > >> > PS: I have another laptop with same version of xen and
> > > > pvops
> > > > > dom0 but
> > > > > > >> > it has ATI radeon graphics card. This laptop boots dom0
> > > > with
> > > > > graphics
> > > > > > >> > when I give nomodeset and nopat options (but fails if I
> > > > dont
> > > > > give
> > > > > > >> > either of those two options).
> > > > > > >> >
> > > > > > >> >
> > > > > > >> >
> > > > > > >> > _______________________________________________
> > > > > > >> > Xen-devel mailing list
> > > > > > >> > [5]Xen-devel@lists.xensource.com<mailto:Xen-devel@lists.xensource.com>
> > > > > > >> > [6]http://lists.xensource.com/xen-devel
> > > > > > >>
> > > > > > >>
> > > > > > >
> > > > >
> > > > > > _______________________________________________
> > > > > > Xen-devel mailing list
> > > > > > [7]Xen-devel@lists.xensource.com<mailto:Xen-devel@lists.xensource.com>
> > > > > > [8]http://lists.xensource.com/xen-devel
> > > > >
> > > > > --
> > > > > ----------------------
> > > > > Dr. Sanjay Kumar
> > > > > Research Scientist
> > > > > Intel Corporation
> > > > >
> > > > > --
> > > > > ----------------------
> > > > > Dr. Sanjay Kumar
> > > > > Research Scientist
> > > > > Intel Corporation
> > > > >
> > > > > References
> > > > >
> > > > > Visible links
> > > > > 1. mailto:sanjay.kushwaha@gmail.com<mailto:sanjay.kushwaha@gmail.com>
> > > > > 2. mailto:konrad.wilk@oracle.com<mailto:konrad.wilk@oracle.com>
> > > > > 3. mailto:sanjay.kushwaha@gmail.com<mailto:sanjay.kushwaha@gmail.com>
> > > > > 4. mailto:jeremy@goop.org<mailto:jeremy@goop.org>
> > > > > 5. mailto:Xen-devel@lists.xensource.com<mailto:Xen-devel@lists.xensource.com>
> > > > > 6. http://lists.xensource.com/xen-devel
> > > > > 7. mailto:Xen-devel@lists.xensource.com<mailto:Xen-devel@lists.xensource.com>
> > > > > 8. http://lists.xensource.com/xen-devel
> > > >
> > > > > _______________________________________________
> > > > > Xen-devel mailing list
> > > > > Xen-devel@lists.xensource.com<mailto:Xen-devel@lists.xensource.com>
> > > > > http://lists.xensource.com/xen-devel
> > > >
> > > >
> > >
> > >
> > > --
> > > ----------------------
> > > Dr. Sanjay Kumar
> > > Research Scientist
> > > Intel Corporation
> >
>
>
>
> --
> ----------------------
> Dr. Sanjay Kumar
> Research Scientist
> Intel Corporation
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com<mailto:Xen-devel@lists.xensource.com>
> http://lists.xensource.com/xen-devel
--
----------------------
Dr. Sanjay Kumar
Research Scientist
Intel Corporation
[-- Attachment #1.2: Type: text/html, Size: 23208 bytes --]
[-- Attachment #2: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply
* Re: [PATCH 01/10] V4L/DVB: cx231xx: remove a printk warning at -avcore and at -417
From: Devin Heitmueller @ 2010-10-07 22:04 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Srinivasa.Deevi, Palash.Bandyopadhyay, Linux Media Mailing List
In-Reply-To: <4CAE4020.4000209@redhat.com>
On Thu, Oct 7, 2010 at 5:48 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Em 28-09-2010 15:46, Mauro Carvalho Chehab escreveu:
>> drivers/media/video/cx231xx/cx231xx-avcore.c:1608: warning: format ‘%d’ expects type ‘int’, but argument 3 has type ‘long unsigned int’
>> drivers/media/video/cx231xx/cx231xx-417.c:1047: warning: format ‘%d’ expects type ‘int’, but argument 3 has type ‘size_t’
>>
>> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>
> OK, I just updated my tree with the patches that Mkrufky acked.
> It basically contains the same patches from my previous post, plus
> the patches that Palash sent, and Devin/Mkrufky patches from polaris4
> tree, rebased over the top of kernel v2.6.36-rc7 (this makes easier
> for me to test and to merge).
>
> The patches are at:
> http://git.linuxtv.org/mchehab/cx231xx.git
>
> Sri already sent his ack for the first series of the patches.
>
> The tree contains two extra patches:
>
> 1) a cx231xx large CodingStyle fix patch:
> http://git.linuxtv.org/mchehab/cx231xx.git?a=commit;h=eacd1a7749ae45d1f2f5782c013b863ff480746d
>
> It basically solves the issues that checkpatch.pl complained on this series of patches;
>
> 2) a cx231xx-417 gcc warning fix:
> http://git.linuxtv.org/mchehab/cx231xx.git?a=commit;h=ca3a6a8c2a4819702e93b9612c4a6d90474ea9b5
>
> Devin,
>
> Would it be ok for you if I merge them on my main tree? They're needed for one
> board I'm working with (a Pixelview SBTVD Hybrid - that supports both analog
> and full-seg ISDB-T).
Yeah, I've got additional fixes which aren't on that tree yet, but I
don't see any reason why what's there cannot be merged.
It would be helpful if you could get Douglas to merge both sets of
patches (mine and yours) to the hg backport tree as well, so I can
continue development without requiring the bleeding edge kernel (all
the work going on is for an embedded target which is running a
relatively old kernel).
I've got another couple dozen patches and I'm willing to continue
pushing this stuff upstream, but you need to meet me halfway here by
not making the bleeding edge kernel a requirement for this work.
Devin
--
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
^ permalink raw reply
* RE: [PATCH] power: introduce library for device-specific OPPs
From: Menon, Nishanth @ 2010-10-07 22:03 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-pm, lkml, l-o, l-a, Paul
In-Reply-To: <201010072354.57011.rjw@sisk.pl>
> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@sisk.pl]
> Sent: Thursday, October 07, 2010 4:55 PM
>
> Hi,
>
> On Wednesday, October 06, 2010, Nishanth Menon wrote:
> > SoCs have a standard set of tuples consisting of frequency and
> > voltage pairs that the device will support per voltage domain. These
> > are called Operating Performance Points or OPPs. The actual
> > definitions of OPP varies over silicon versions. For a specific domain,
> > we can have a set of {frequency, voltage} pairs. As the kernel boots
> > and more information is available, a default set of these are activated
> > based on the precise nature of device. Further on operation, based on
> > conditions prevailing in the system (such as temperature), some OPP
> > availability may be temporarily controlled by the SoC frameworks.
> >
> > To implement an OPP, some sort of power management support is necessary
> > hence this library depends on CONFIG_PM.
>
> The patch generally looks good to me, I only have a couple of cosmetic
Thanks for the great reviews.. It did bump up the resultant patch.
> remarks
> (below).
>
> ...
> > +static int opp_set_availability(struct device *dev, unsigned long freq,
> > + bool availability_req)
> > +{
> > + struct device_opp *tmp_dev_opp, *dev_opp = NULL;
> > + struct opp *new_opp, *tmp_opp, *opp = ERR_PTR(-ENODEV);
> > + int r = 0;
> > +
> > + /* keep the node allocated */
> > + new_opp = kmalloc(sizeof(struct opp), GFP_KERNEL);
> > + if (!new_opp) {
> > + pr_warning("Unable to allocate opp\n");
>
> Please add an identification string to the messages, something like
> "OPP: Unable to allocat object\n" (and similarly in the other messages).
> That would help to find the source of a message in case there's any
> problem.
pr_fmt has been reformatted for this. The actual message which will appear
is as follows:
opp_set_availability: Unable to allocate opp
is'nt that good enough considering that all functions are opp_ prefixed?
I can modify pr_fmt to add "OPP:" but I kinda think it is redundant. But I
have no strong opinions on that and look forward to your recommendations.
>
>
> > + return -ENOMEM;
> > + }
> > +
> > + mutex_lock(&dev_opp_list_lock);
> > +
> > + /* Find the device_opp */
> > + list_for_each_entry(tmp_dev_opp, &dev_opp_list, node) {
> > + if (dev == tmp_dev_opp->dev) {
> > + dev_opp = tmp_dev_opp;
> > + break;
> > + }
> > + }
> > + if (IS_ERR(dev_opp)) {
> > + r = PTR_ERR(dev_opp);
> > + pr_warning("Unable to find device\n");
> > + goto out1;
>
> I'd prefer this lable to be called "unlock". It will be a bit more
> informative.
Ack. Fixing in v6 if you are ok with the above.
Regards,
Nishanth Menon
^ permalink raw reply
* [PATCH] power: introduce library for device-specific OPPs
From: Menon, Nishanth @ 2010-10-07 22:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <201010072354.57011.rjw@sisk.pl>
> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw at sisk.pl]
> Sent: Thursday, October 07, 2010 4:55 PM
>
> Hi,
>
> On Wednesday, October 06, 2010, Nishanth Menon wrote:
> > SoCs have a standard set of tuples consisting of frequency and
> > voltage pairs that the device will support per voltage domain. These
> > are called Operating Performance Points or OPPs. The actual
> > definitions of OPP varies over silicon versions. For a specific domain,
> > we can have a set of {frequency, voltage} pairs. As the kernel boots
> > and more information is available, a default set of these are activated
> > based on the precise nature of device. Further on operation, based on
> > conditions prevailing in the system (such as temperature), some OPP
> > availability may be temporarily controlled by the SoC frameworks.
> >
> > To implement an OPP, some sort of power management support is necessary
> > hence this library depends on CONFIG_PM.
>
> The patch generally looks good to me, I only have a couple of cosmetic
Thanks for the great reviews.. It did bump up the resultant patch.
> remarks
> (below).
>
> ...
> > +static int opp_set_availability(struct device *dev, unsigned long freq,
> > + bool availability_req)
> > +{
> > + struct device_opp *tmp_dev_opp, *dev_opp = NULL;
> > + struct opp *new_opp, *tmp_opp, *opp = ERR_PTR(-ENODEV);
> > + int r = 0;
> > +
> > + /* keep the node allocated */
> > + new_opp = kmalloc(sizeof(struct opp), GFP_KERNEL);
> > + if (!new_opp) {
> > + pr_warning("Unable to allocate opp\n");
>
> Please add an identification string to the messages, something like
> "OPP: Unable to allocat object\n" (and similarly in the other messages).
> That would help to find the source of a message in case there's any
> problem.
pr_fmt has been reformatted for this. The actual message which will appear
is as follows:
opp_set_availability: Unable to allocate opp
is'nt that good enough considering that all functions are opp_ prefixed?
I can modify pr_fmt to add "OPP:" but I kinda think it is redundant. But I
have no strong opinions on that and look forward to your recommendations.
>
>
> > + return -ENOMEM;
> > + }
> > +
> > + mutex_lock(&dev_opp_list_lock);
> > +
> > + /* Find the device_opp */
> > + list_for_each_entry(tmp_dev_opp, &dev_opp_list, node) {
> > + if (dev == tmp_dev_opp->dev) {
> > + dev_opp = tmp_dev_opp;
> > + break;
> > + }
> > + }
> > + if (IS_ERR(dev_opp)) {
> > + r = PTR_ERR(dev_opp);
> > + pr_warning("Unable to find device\n");
> > + goto out1;
>
> I'd prefer this lable to be called "unlock". It will be a bit more
> informative.
Ack. Fixing in v6 if you are ok with the above.
Regards,
Nishanth Menon
^ permalink raw reply
* Re: [PATCH] power: introduce library for device-specific OPPs
From: Menon, Nishanth @ 2010-10-07 22:03 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Paul, linux-pm, l-o, lkml, l-a
In-Reply-To: <201010072354.57011.rjw@sisk.pl>
> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@sisk.pl]
> Sent: Thursday, October 07, 2010 4:55 PM
>
> Hi,
>
> On Wednesday, October 06, 2010, Nishanth Menon wrote:
> > SoCs have a standard set of tuples consisting of frequency and
> > voltage pairs that the device will support per voltage domain. These
> > are called Operating Performance Points or OPPs. The actual
> > definitions of OPP varies over silicon versions. For a specific domain,
> > we can have a set of {frequency, voltage} pairs. As the kernel boots
> > and more information is available, a default set of these are activated
> > based on the precise nature of device. Further on operation, based on
> > conditions prevailing in the system (such as temperature), some OPP
> > availability may be temporarily controlled by the SoC frameworks.
> >
> > To implement an OPP, some sort of power management support is necessary
> > hence this library depends on CONFIG_PM.
>
> The patch generally looks good to me, I only have a couple of cosmetic
Thanks for the great reviews.. It did bump up the resultant patch.
> remarks
> (below).
>
> ...
> > +static int opp_set_availability(struct device *dev, unsigned long freq,
> > + bool availability_req)
> > +{
> > + struct device_opp *tmp_dev_opp, *dev_opp = NULL;
> > + struct opp *new_opp, *tmp_opp, *opp = ERR_PTR(-ENODEV);
> > + int r = 0;
> > +
> > + /* keep the node allocated */
> > + new_opp = kmalloc(sizeof(struct opp), GFP_KERNEL);
> > + if (!new_opp) {
> > + pr_warning("Unable to allocate opp\n");
>
> Please add an identification string to the messages, something like
> "OPP: Unable to allocat object\n" (and similarly in the other messages).
> That would help to find the source of a message in case there's any
> problem.
pr_fmt has been reformatted for this. The actual message which will appear
is as follows:
opp_set_availability: Unable to allocate opp
is'nt that good enough considering that all functions are opp_ prefixed?
I can modify pr_fmt to add "OPP:" but I kinda think it is redundant. But I
have no strong opinions on that and look forward to your recommendations.
>
>
> > + return -ENOMEM;
> > + }
> > +
> > + mutex_lock(&dev_opp_list_lock);
> > +
> > + /* Find the device_opp */
> > + list_for_each_entry(tmp_dev_opp, &dev_opp_list, node) {
> > + if (dev == tmp_dev_opp->dev) {
> > + dev_opp = tmp_dev_opp;
> > + break;
> > + }
> > + }
> > + if (IS_ERR(dev_opp)) {
> > + r = PTR_ERR(dev_opp);
> > + pr_warning("Unable to find device\n");
> > + goto out1;
>
> I'd prefer this lable to be called "unlock". It will be a bit more
> informative.
Ack. Fixing in v6 if you are ok with the above.
Regards,
Nishanth Menon
^ permalink raw reply
* RE: [PATCH] power: introduce library for device-specific OPPs
From: Menon, Nishanth @ 2010-10-07 22:03 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Paul, linux-pm, l-o, lkml, l-a
In-Reply-To: <201010072354.57011.rjw@sisk.pl>
> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@sisk.pl]
> Sent: Thursday, October 07, 2010 4:55 PM
>
> Hi,
>
> On Wednesday, October 06, 2010, Nishanth Menon wrote:
> > SoCs have a standard set of tuples consisting of frequency and
> > voltage pairs that the device will support per voltage domain. These
> > are called Operating Performance Points or OPPs. The actual
> > definitions of OPP varies over silicon versions. For a specific domain,
> > we can have a set of {frequency, voltage} pairs. As the kernel boots
> > and more information is available, a default set of these are activated
> > based on the precise nature of device. Further on operation, based on
> > conditions prevailing in the system (such as temperature), some OPP
> > availability may be temporarily controlled by the SoC frameworks.
> >
> > To implement an OPP, some sort of power management support is necessary
> > hence this library depends on CONFIG_PM.
>
> The patch generally looks good to me, I only have a couple of cosmetic
Thanks for the great reviews.. It did bump up the resultant patch.
> remarks
> (below).
>
> ...
> > +static int opp_set_availability(struct device *dev, unsigned long freq,
> > + bool availability_req)
> > +{
> > + struct device_opp *tmp_dev_opp, *dev_opp = NULL;
> > + struct opp *new_opp, *tmp_opp, *opp = ERR_PTR(-ENODEV);
> > + int r = 0;
> > +
> > + /* keep the node allocated */
> > + new_opp = kmalloc(sizeof(struct opp), GFP_KERNEL);
> > + if (!new_opp) {
> > + pr_warning("Unable to allocate opp\n");
>
> Please add an identification string to the messages, something like
> "OPP: Unable to allocat object\n" (and similarly in the other messages).
> That would help to find the source of a message in case there's any
> problem.
pr_fmt has been reformatted for this. The actual message which will appear
is as follows:
opp_set_availability: Unable to allocate opp
is'nt that good enough considering that all functions are opp_ prefixed?
I can modify pr_fmt to add "OPP:" but I kinda think it is redundant. But I
have no strong opinions on that and look forward to your recommendations.
>
>
> > + return -ENOMEM;
> > + }
> > +
> > + mutex_lock(&dev_opp_list_lock);
> > +
> > + /* Find the device_opp */
> > + list_for_each_entry(tmp_dev_opp, &dev_opp_list, node) {
> > + if (dev == tmp_dev_opp->dev) {
> > + dev_opp = tmp_dev_opp;
> > + break;
> > + }
> > + }
> > + if (IS_ERR(dev_opp)) {
> > + r = PTR_ERR(dev_opp);
> > + pr_warning("Unable to find device\n");
> > + goto out1;
>
> I'd prefer this lable to be called "unlock". It will be a bit more
> informative.
Ack. Fixing in v6 if you are ok with the above.
Regards,
Nishanth Menon
^ permalink raw reply
* Re: [PATCH] serial: DCC(JTAG) serial and console emulation support
From: Greg KH @ 2010-10-07 21:52 UTC (permalink / raw)
To: Daniel Walker
Cc: Alan Cox, linux-kernel, Hyok S. Choi, Tony Lindgren,
Jeff Ohlstein, Ben Dooks, Alan Cox, Kukjin Kim, Mike Frysinger,
Feng Tang, Tobias Klauser, Jason Wessel, Philippe Langlais
In-Reply-To: <1286488038.23836.101.camel@c-dwalke-linux.qualcomm.com>
On Thu, Oct 07, 2010 at 02:47:18PM -0700, Daniel Walker wrote:
> On Thu, 2010-10-07 at 14:15 -0700, Greg KH wrote:
> > On Thu, Oct 07, 2010 at 01:51:18PM -0700, Daniel Walker wrote:
> > > Your making too many assumptions .. You might be able to modify the
> > > kernel, and not the userspace. So you couldn't tweak the device
> > > creation .. It's much easier in the server world ..
> >
> > You're saying it's easier to replace an embedded kernel than a userspace
> > file on an embedded system? Heh, that's funny.
>
> I'm saying it _can_ happen that it's easier to replace the kernel.. I'm
> not saying it's always the case. We're talking about debugging
> situations in an embedded environment which don't always follow a normal
> path.
Nothing is "normal", so stop trying to say it is easier in the "server"
world please.
> > > > We've said no over a period of about ten years to a lot of attempts to
> > > > just borrow the ttyS0 range. If we'd said yes it would have been a
> > > > complete mess by now.
> > > >
> > > > So the answer is no.
> > >
> > > Nothing can be unilateral, there's always room for exceptions. You
> > > should say something more like "it's possible, but unlikely".
> >
> > Hm, how about this, as the TTY and serial driver[1] maintainer, I will
> > not accept this kind of patch at all.
> >
> > Is that final enough for you?
>
> So you don't like it, that's fair enough .. <thinks>I wonder what other
> maintainers I can send this too</thinks> ;)
>
> Can you be more specific about your objections
See everything that Mike and Alan wrote, do I need to repeat it?
greg k-h
^ permalink raw reply
* Re: memory clobber in rx path, maybe related to ath9k.
From: Luis R. Rodriguez @ 2010-10-07 21:59 UTC (permalink / raw)
To: Johannes Berg; +Cc: Ben Greear, linux-wireless@vger.kernel.org
In-Reply-To: <AANLkTi=t-P_5c14brJohM8UgxeOknv+3syO=7W=ix6qf@mail.gmail.com>
On Thu, Oct 7, 2010 at 2:36 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> On Thu, Oct 7, 2010 at 2:31 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
>> On Thu, Oct 7, 2010 at 12:27 PM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>>> On Thu, 2010-10-07 at 12:22 -0700, Ben Greear wrote:
>>>
>>>> After reboot, and re-run of the script,
>>>> I saw this in the logs, and shortly after,
>>>> the SLUB poison warning dumped to screen.
>>>>
>>>> Maybe those DMA errors are serious?
>>>
>>>> ath: Failed to stop TX DMA in 100 msec after killing last frame
>>>> ath: Failed to stop TX DMA. Resetting hardware!
>>>
>>> That's TX DMA, it can hardly result in invalid memory writes like the
>>> ones you've been seeing.
>>>
>>> I'm still convinced something is wrong with ath9k RX DMA, as you've seen
>>> the contents of frames written to already freed memory regions. Since I
>>> don't know anything about ath9k, you should probably not rely on me
>>> though :-)
>>
>> I'm on this now. Lets play.
>>
>> I had to remove /lib/udev/rules.d/75-persistent-net-generator.rules
>> to avoid Ubuntu trying to remember the device names and it creating
>> stax_rename names.
>> I just ran your script with some modifications. You can find it here:
>>
>> http://www.kernel.org/pub/linux/kernel/people/mcgrof/scripts/poo.pl
>>
>> I then ran:
>>
>> for i in $(seq 0 31) ; do sudo dhclient seq$i; done
>>
>> It took about 10 minutes to get IP addresses for all interfaces but it
>> got there eventually. Odd enough I was unable to ping the AP from any
>> interface though. Not sure what that was about. But I got no oops, no
>> slub dump. All I got was some more delba warnings which seems to
>> indicate we haven't caught all the cases for its use:
>>
>> [ 3622.660344] addBA response timer expired on tid 0
>> [ 3622.660373] Tx BA session stop requested for 68:7f:74:3b:b1:0f tid 0
>> [ 3622.680133] addBA response timer expired on tid 0
>> [ 3622.687196] Tx BA session stop requested for 68:7f:74:3b:b1:0f tid 0
>> [ 3623.110077] addBA response timer expired on tid 0
>> [ 3623.110123] Tx BA session stop requested for 68:7f:74:3b:b1:0f tid 0
>> [ 3628.935895] sta10: authenticate with 68:7f:74:3b:b1:10 (try 1)
>> [ 3628.937194] switched off addBA timer for tid 0
>> [ 3628.937196] Aggregation is on for tid 0
>> [ 3628.937239] Stopping Tx BA session for 68:7f:74:3b:b1:0f tid 0
>> [ 3628.937243] ------------[ cut here ]------------
>> [ 3628.937263] WARNING: at include/net/mac80211.h:2694
>> rate_control_send_low+0xd3/0x140 [mac80211]()
>> [ 3628.937265] Hardware name: 6460DWU
>> [ 3628.937266] Modules linked in: binfmt_misc ppdev
>> snd_hda_codec_analog rfcomm sco bridge joydev stp bnep l2cap nouveau
>> ath9k snd_hda_intel mac80211 snd_hda_codec snd_hwdep snd_pcm ttm btusb
>> ath9k_common thinkpad_acpi ath9k_hw bluetooth drm_kms_helper
>> snd_seq_midi snd_rawmidi pcmcia snd_seq_midi_event drm snd_seq ath
>> snd_timer snd_seq_device tpm_tis i2c_algo_bit cfg80211 snd nvram tpm
>> tpm_bios yenta_socket pcmcia_rsrc video psmouse output pcmcia_core
>> serio_raw soundcore snd_page_alloc intel_agp lp parport ohci1394
>> e1000e ieee1394 ahci libahci
>> [ 3628.937307] Pid: 49, comm: kworker/u:3 Tainted: G W
>> 2.6.36-rc6-wl+ #263
>> [ 3628.937310] Call Trace:
>> [ 3628.937317] [<ffffffff8105ffcf>] warn_slowpath_common+0x7f/0xc0
>> [ 3628.937320] [<ffffffff8106002a>] warn_slowpath_null+0x1a/0x20
>> [ 3628.937329] [<ffffffffa032f603>] rate_control_send_low+0xd3/0x140 [mac80211]
>> [ 3628.937336] [<ffffffffa038bfd8>] ath_get_rate+0x48/0x570 [ath9k]
>> [ 3628.937340] [<ffffffff812b9f39>] ? put_dec+0x59/0x60
>> [ 3628.937349] [<ffffffffa032f42e>] rate_control_get_rate+0x8e/0x190 [mac80211]
>> [ 3628.937360] [<ffffffffa0339928>]
>> ieee80211_tx_h_rate_ctrl+0x1a8/0x4e0 [mac80211]
>> [ 3628.937370] [<ffffffffa033a000>] invoke_tx_handlers+0x100/0x140 [mac80211]
>> [ 3628.937379] [<ffffffffa033a0c5>] ieee80211_tx+0x85/0x240 [mac80211]
>> [ 3628.937384] [<ffffffff8147b890>] ? skb_release_data+0xd0/0xe0
>> [ 3628.937386] [<ffffffff8147d72f>] ? pskb_expand_head+0x10f/0x1a0
>> [ 3628.937397] [<ffffffffa033a336>] ieee80211_xmit+0xb6/0x1d0 [mac80211]
>> [ 3628.937399] [<ffffffff8147b9d3>] ? __alloc_skb+0x83/0x170
>> [ 3628.937409] [<ffffffffa033a4a4>] ieee80211_tx_skb+0x54/0x70 [mac80211]
>> [ 3628.937418] [<ffffffffa03230ad>] ieee80211_send_delba+0x11d/0x190 [mac80211]
>> [ 3628.937427] [<ffffffffa0323a18>]
>> ieee80211_stop_tx_ba_cb+0x1b8/0x240 [mac80211]
>> [ 3628.937431] [<ffffffff81036c89>] ? default_spin_lock_flags+0x9/0x10
>> [ 3628.937440] [<ffffffffa032e031>] ieee80211_iface_work+0x271/0x340 [mac80211]
>> [ 3628.937450] [<ffffffffa032ddc0>] ? ieee80211_iface_work+0x0/0x340 [mac80211]
>> [ 3628.937453] [<ffffffff8107a203>] process_one_work+0x123/0x440
>> [ 3628.937457] [<ffffffff8107c750>] worker_thread+0x170/0x400
>> [ 3628.937460] [<ffffffff8107c5e0>] ? worker_thread+0x0/0x400
>> [ 3628.937463] [<ffffffff81080b76>] kthread+0x96/0xa0
>> [ 3628.937466] [<ffffffff8100bea4>] kernel_thread_helper+0x4/0x10
>> [ 3628.937469] [<ffffffff81080ae0>] ? kthread+0x0/0xa0
>> [ 3628.937472] [<ffffffff8100bea0>] ? kernel_thread_helper+0x0/0x10
>> [ 3628.937474] ---[ end trace 9dd0d025ccb9b75c ]---
>> [ 3628.937980] switched off addBA timer for tid 0
>> [ 3628.937982] Aggregation is on for tid 0
>>
>> But other than this I got nothing. I left the box sit there for about
>> 1 hour and came back and it was still going with no issues. Mind you,
>> I can't ping but that seems like another issue.
>>
>> You can find my logs here:
>>
>> http://www.kernel.org/pub/linux/kernel/people/mcgrof/logs/2010/10-07-stress-sta-01/
>
> Doh, I did not have CONFIG_SLUB_DEBUG_ON=y so building now with that.
Yay I can reproduce now. I'll be back, going to dig now.
Luis
^ permalink raw reply
* + percpu_counter-use-this_cpu_ptr-instead-of-per_cpu_ptr.patch added to -mm tree
From: akpm @ 2010-10-07 21:59 UTC (permalink / raw)
To: mm-commits; +Cc: cl, eric.dumazet, tj
The patch titled
percpu_counter: use this_cpu_ptr() instead of per_cpu_ptr()
has been added to the -mm tree. Its filename is
percpu_counter-use-this_cpu_ptr-instead-of-per_cpu_ptr.patch
Before you just go and hit "reply", please:
a) Consider who else should be cc'ed
b) Prefer to cc a suitable mailing list as well
c) Ideally: find the original patch on the mailing list and do a
reply-to-all to that, adding suitable additional cc's
*** Remember to use Documentation/SubmitChecklist when testing your code ***
See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
out what to do about this
The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/
------------------------------------------------------
Subject: percpu_counter: use this_cpu_ptr() instead of per_cpu_ptr()
From: Christoph Lameter <cl@linux.com>
this_cpu_ptr() avoids an array lookup and can use the percpu offset of the
local cpu directly.
Signed-off-by: Christoph Lameter <cl@linux.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
lib/percpu_counter.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff -puN lib/percpu_counter.c~percpu_counter-use-this_cpu_ptr-instead-of-per_cpu_ptr lib/percpu_counter.c
--- a/lib/percpu_counter.c~percpu_counter-use-this_cpu_ptr-instead-of-per_cpu_ptr
+++ a/lib/percpu_counter.c
@@ -73,9 +73,9 @@ void __percpu_counter_add(struct percpu_
{
s64 count;
s32 *pcount;
- int cpu = get_cpu();
- pcount = per_cpu_ptr(fbc->counters, cpu);
+ preempt_disable();
+ pcount = this_cpu_ptr(fbc->counters);
count = *pcount + amount;
if (count >= batch || count <= -batch) {
spin_lock(&fbc->lock);
@@ -85,7 +85,7 @@ void __percpu_counter_add(struct percpu_
} else {
*pcount = count;
}
- put_cpu();
+ preempt_enable();
}
EXPORT_SYMBOL(__percpu_counter_add);
_
Patches currently in -mm which might be from cl@linux.com are
linux-next.patch
mm-compaction-fix-compactpagefailed-counting.patch
mm-mempolicy-check-return-code-of-check_range.patch
percpu_counter-use-this_cpu_ptr-instead-of-per_cpu_ptr.patch
^ permalink raw reply
* Re: 2.6.36-rc7 warning in __cfg80211_auth_remove
From: Johannes Stezenbach @ 2010-10-07 21:59 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless
In-Reply-To: <1286482766.20974.39.camel@jlt3.sipsolutions.net>
On Thu, Oct 07, 2010 at 10:19:26PM +0200, Johannes Berg wrote:
> On Thu, 2010-10-07 at 17:18 +0200, Johannes Stezenbach wrote:
> >
> > just got the following while testing with rt73usb on
> > kernel 2.6.36-rc7 with wpa_supplicant v0.6.10:
> >
> > wlan0: authentication with 00:22:b0:xx:xx:xx timed out
> > ------------[ cut here ]------------
> > WARNING: at net/wireless/mlme.c:284 __cfg80211_auth_remove+0xc3/0xd0 [cfg80211]()
>
> > [<ffffffffa002d980>] __cfg80211_auth_remove+0xc3/0xd0 [cfg80211]
> > [<ffffffffa002e27c>] cfg80211_send_auth_timeout+0x93/0xaa [cfg80211]
> > [<ffffffffa004c3eb>] ieee80211_probe_auth_done+0x2a/0x97 [mac80211]
> > [<ffffffffa004e551>] ieee80211_work_work+0xecd/0xf3e [mac80211]
>
> Oh *crap*. I thought we'd fixed these :-(
>
> But then I really don't see how it might have happened. Was there
> anything in the log before this?
Nothing interesting. I started and ^C'ed wpa_supplicant
a few times in quick succession for testing because of
http://rt2x00.serialmonkey.com/pipermail/users_rt2x00.serialmonkey.com/2010-October/002152.html
Johannes
^ permalink raw reply
* [PATCH 1/2] ASoC: Add CS4271 codec support
From: Ryan Mallon @ 2010-10-07 21:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <4CAE3FAC.6010508@freescale.com>
On 10/08/2010 10:46 AM, Timur Tabi wrote:
> Ryan Mallon wrote:
>> + * For setups with variable MCLKs, pass 0 as 'freq' argument. This will cause
>> + * theoretically possible sample rates to be enabled. Call it again with a
>> + * proper value set one the external clock is set (most probably you would do
>> + * that from a machine's driver 'hw_param' hook.
>
> If you're going to copy/paste parts of my driver verbatim into yours, you should
> put something like this in the comments:
>
> Based on the CS4270 driver by Timur Tabi <timur@freescale.com>
>
Thanks. CC'ed Alexander, the driver author, who I stupidly left of the
reply list.
~Ryan
--
Bluewater Systems Ltd - ARM Technology Solution Centre
Ryan Mallon 5 Amuri Park, 404 Barbadoes St
ryan at bluewatersys.com PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com New Zealand
Phone: +64 3 3779127 Freecall: Australia 1800 148 751
Fax: +64 3 3779135 USA 1800 261 2934
^ permalink raw reply
* Re: [PATCH] dell-laptop: Add hwswitch_only module parameter
From: Dmitry Torokhov @ 2010-10-07 21:58 UTC (permalink / raw)
To: Matthew Garrett
Cc: Mario Limonciello, Keng-Yu Lin, len.brown, alan-jenkins,
platform-driver-x86, linux-kernel
In-Reply-To: <20101007215028.GB2292@srcf.ucam.org>
On Thu, Oct 07, 2010 at 10:50:28PM +0100, Matthew Garrett wrote:
> On Thu, Oct 07, 2010 at 02:46:28PM -0700, Dmitry Torokhov wrote:
> > On Thu, Oct 07, 2010 at 10:37:54PM +0100, Matthew Garrett wrote:
> > > Dell laptop driver is reasonable. All we need is a list of hardware, and
> > > I'd really hope that Dell know which BIOS versions contain this code!
> > >
> >
> > Unfortunately this kind of crap tends to flow from one BIOS version to
> > another.. How many DMI entries do we know about so far? Did Dell issue
> > firmware updates correcting the issue so new boxes will not exhibit the
> > problem?
>
> Pushing this out to userspace doesn't help. If it's sufficiently
> prevelant that it's impossible to have a comprehensive list, then a
> module option makes it too easy for people to just post the fix on a
> forum somewhere and never actually get it upstream.
>
Forcing users patching the kernel reduces chances that it will be fixed
at all. Also there is load of patches that never make into distributions
and even less in mainline. Or if they make it it takes looong time...
so I do not think it is a valid argument here.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] ASoC: Add CS4271 codec support
From: Ryan Mallon @ 2010-10-07 21:59 UTC (permalink / raw)
To: Timur Tabi
Cc: alsa-devel@alsa-project.org, Mark Brown, Alexander,
linux-arm-kernel, Liam Girdwood
In-Reply-To: <4CAE3FAC.6010508@freescale.com>
On 10/08/2010 10:46 AM, Timur Tabi wrote:
> Ryan Mallon wrote:
>> + * For setups with variable MCLKs, pass 0 as 'freq' argument. This will cause
>> + * theoretically possible sample rates to be enabled. Call it again with a
>> + * proper value set one the external clock is set (most probably you would do
>> + * that from a machine's driver 'hw_param' hook.
>
> If you're going to copy/paste parts of my driver verbatim into yours, you should
> put something like this in the comments:
>
> Based on the CS4270 driver by Timur Tabi <timur@freescale.com>
>
Thanks. CC'ed Alexander, the driver author, who I stupidly left of the
reply list.
~Ryan
--
Bluewater Systems Ltd - ARM Technology Solution Centre
Ryan Mallon 5 Amuri Park, 404 Barbadoes St
ryan@bluewatersys.com PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com New Zealand
Phone: +64 3 3779127 Freecall: Australia 1800 148 751
Fax: +64 3 3779135 USA 1800 261 2934
^ permalink raw reply
* Re: [PATCH] serial: DCC(JTAG) serial and console emulation support
From: Daniel Walker @ 2010-10-07 21:58 UTC (permalink / raw)
To: Tony Lindgren
Cc: linux-arm-kernel, linux-arm-msm, Hyok S. Choi, Jeff Ohlstein
In-Reply-To: <20101007212707.GI7558@atomide.com>
On Thu, 2010-10-07 at 14:27 -0700, Tony Lindgren wrote:
> Can you please pass the read and write functions to the driver
> in platform_data? We are already booting kernels with both
> ARMv6 and 7 compiled in.
What kind of situation did you want to use it in ? I was thinking about
how arm could have these functions in a header file, that way we
wouldn't duplicate in multiple places.
> Also, as it's a driver, other architectures may want to use it too.
I'm not sure if this model fits other architectures tho. They may not
have the same read/write/status sorts of stuff. Atleast I don't know
enough about other architectures jtags .
Daniel
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply
* [PATCH] serial: DCC(JTAG) serial and console emulation support
From: Daniel Walker @ 2010-10-07 21:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20101007212707.GI7558@atomide.com>
On Thu, 2010-10-07 at 14:27 -0700, Tony Lindgren wrote:
> Can you please pass the read and write functions to the driver
> in platform_data? We are already booting kernels with both
> ARMv6 and 7 compiled in.
What kind of situation did you want to use it in ? I was thinking about
how arm could have these functions in a header file, that way we
wouldn't duplicate in multiple places.
> Also, as it's a driver, other architectures may want to use it too.
I'm not sure if this model fits other architectures tho. They may not
have the same read/write/status sorts of stuff. Atleast I don't know
enough about other architectures jtags .
Daniel
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply
* Re: [PATCH] dell-laptop: Add hwswitch_only module parameter
From: Mario Limonciello @ 2010-10-07 21:57 UTC (permalink / raw)
To: Matthew Garrett
Cc: Dmitry Torokhov, Keng-Yu Lin, len.brown, alan-jenkins,
platform-driver-x86, linux-kernel
In-Reply-To: <20101007215028.GB2292@srcf.ucam.org>
On Thu, Oct 7, 2010 at 16:50, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Thu, Oct 07, 2010 at 02:46:28PM -0700, Dmitry Torokhov wrote:
>> On Thu, Oct 07, 2010 at 10:37:54PM +0100, Matthew Garrett wrote:
>> > Dell laptop driver is reasonable. All we need is a list of hardware, and
>> > I'd really hope that Dell know which BIOS versions contain this code!
>> >
>>
>> Unfortunately this kind of crap tends to flow from one BIOS version to
>> another.. How many DMI entries do we know about so far? Did Dell issue
>> firmware updates correcting the issue so new boxes will not exhibit the
>> problem?
>
> Pushing this out to userspace doesn't help. If it's sufficiently
> prevelant that it's impossible to have a comprehensive list, then a
> module option makes it too easy for people to just post the fix on a
> forum somewhere and never actually get it upstream.
>
> --
> Matthew Garrett | mjg59@srcf.ucam.org
>
Making it more difficult to enable the alternative functionality
wouldn't necessarily make the list any more comprehensive, it just
raises the bar for users who want to try to help. If users report the
bugs to their distros, distros will help to forward them upstream.
You can't expect to fix a social problem here just by making it more
difficult to fix the original problem at hand for individuals.
--
Mario Limonciello
superm1@gmail.com
^ permalink raw reply
* Re: ext4 support on pvgrub
From: Samuel Thibault @ 2010-10-07 21:56 UTC (permalink / raw)
To: M A Young; +Cc: xen-devel
In-Reply-To: <alpine.LFD.2.00.1010072238110.13579@vega1.dur.ac.uk>
M A Young, le Thu 07 Oct 2010 22:48:04 +0100, a écrit :
> I have this working on Fedora. It turned out to be very easy to add this
> as I simply took the grub-ext4-support.patch from the Fedora grub package
> and put it in xen-4.0.1/stubdom/grub.patches/ . Using pvgrub (without a
> vif on the virtual machine as I was having problems if I did give it a
> network interface) I could read and boot from ext4 partition with this
> patch but not without it.
>
> Is this a good way to add this feature to xen more generally?
Yes, it's definitely the way to go. I haven't reviewed the patch at
all, but since that's just what Fedora uses it's probably simply ok.
Samuel
^ permalink raw reply
* Re: [Qemu-devel] [PATCH] ceph/rbd block driver for qemu-kvm (v4)
From: Anthony Liguori @ 2010-10-07 21:55 UTC (permalink / raw)
To: Yehuda Sadeh Weinraub
Cc: Kevin Wolf, ceph-devel, qemu-devel, kvm, Christian Brunner
In-Reply-To: <AANLkTinzdiJZFxacD1BDCPWt44ChokNaNVU6_E=GtcTD@mail.gmail.com>
On 10/07/2010 04:49 PM, Yehuda Sadeh Weinraub wrote:
> On Thu, Oct 7, 2010 at 2:04 PM, Anthony Liguori<anthony@codemonkey.ws> wrote:
>
>> On 10/07/2010 03:47 PM, Yehuda Sadeh Weinraub wrote:
>>
>>>> How is that possible? Are the callbacks delivered in the context of a
>>>> different thread? If so, don't you need locking?
>>>>
>>>>
>>> Not sure I'm completely following you. The callbacks are delivered in
>>> the context of a different thread, but won't run concurrently.
>>>
>> Concurrently to what? How do you prevent them from running concurrently
>> with qemu?
>>
> There are two types of callbacks. The first is for rados aio
> completions, and the second one is the one added later for the fd glue
> layer.
>
This is a bad architecture for something like qemu. You could create a
pipe and use the pipe to signal to qemu. Same principle as eventfd.
Ideally, you would do this in the library itself.
Regards,
Anthony Liguori
> The first callback, called by librados whenever aio completes, runs in
> the context of a single librados thread:
>
> +static void rbd_finish_aiocb(rados_completion_t c, RADOSCB *rcb)
> +{
> + RBDAIOCB *acb = rcb->acb;
> rcb is per a single aio. Was created before and will be destroyed
> here, whereas acb is shared between a few aios, however, it was
> generated before the first aio was created.
>
> + int64_t r;
> + uint64_t buf = 1;
> + int i;
> +
> + acb->aiocnt--;
>
> acb->aiocnt has been set before initiating all the aios, so it's ok to
> touch it now. Same goes to all acb fields.
>
> + r = rados_aio_get_return_value(c);
> + rados_aio_release(c);
> + if (acb->write) {
> + if (r< 0) {
> + acb->ret = r;
> + acb->error = 1;
> + } else if (!acb->error) {
> + acb->ret += rcb->segsize;
> + }
> + } else {
> + if (r == -ENOENT) {
> + memset(rcb->buf, 0, rcb->segsize);
> + if (!acb->error) {
> + acb->ret += rcb->segsize;
> + }
> + } else if (r< 0) {
> + acb->ret = r;
> + acb->error = 1;
> + } else if (r< rcb->segsize) {
> + memset(rcb->buf + r, 0, rcb->segsize - r);
> + if (!acb->error) {
> + acb->ret += rcb->segsize;
> + }
> + } else if (!acb->error) {
> + acb->ret += r;
> + }
> + }
> + if (write(acb->s->efd,&buf, sizeof(buf))< 0)
> This will wake up the io_read()
>
> + error_report("failed writing to acb->s->efd\n");
> + qemu_free(rcb);
> + i = 0;
> + if (!acb->aiocnt&& acb->bh) {
> + qemu_bh_schedule(acb->bh);
> This is the only qemu related call in here, seems safe to call it.
>
> + }
> +}
>
> The scheduled bh function will be called only after all aios that
> relate to this specific aio set are done, so the following seems ok,
> as there's no more acb references.
> +static void rbd_aio_bh_cb(void *opaque)
> +{
> + RBDAIOCB *acb = opaque;
> + uint64_t buf = 1;
> +
> + if (!acb->write) {
> + qemu_iovec_from_buffer(acb->qiov, acb->bounce, acb->qiov->size);
> + }
> + qemu_vfree(acb->bounce);
> + acb->common.cb(acb->common.opaque, (acb->ret> 0 ? 0 : acb->ret));
> + qemu_bh_delete(acb->bh);
> + acb->bh = NULL;
> +
> + if (write(acb->s->efd,&buf, sizeof(buf))< 0)
> + error_report("failed writing to acb->s->efd\n");
> + qemu_aio_release(acb);
> +}
>
> Now, the second ones are the io_read(), in which we have our glue fd.
> We send uint64 per each completed io
>
> +static void rbd_aio_completion_cb(void *opaque)
> +{
> + BDRVRBDState *s = opaque;
> +
> + uint64_t val;
> + ssize_t ret;
> +
> + do {
> + if ((ret = read(s->efd,&val, sizeof(val)))> 0) {
> + s->qemu_aio_count -= val;
> There is an issue here with s->qemu_aio_count which needs to be
> protected by a mutex. Other than that, it just reads from s->efd.
>
> + }
> + } while (ret< 0&& errno == EINTR);
> +
> + return;
> +}
> +
> +static int rbd_aio_flush_cb(void *opaque)
> +{
> + BDRVRBDState *s = opaque;
> +
> + return (s->qemu_aio_count> 0);
> Same here as with the previous one, needs a mutex around s->qemu_aio_count.
>
> +}
>
>
>> If you saw lock ups, I bet that's what it was from.
>>
>>
> As I explained before, before introducing the fd glue layer, the lack
> of fd associated with our block device caused that there was no way
> for qemu to check whether all aios were flushed or not, which didn't
> work well when doing migration/savevm.
>
> Thanks,
> Yehuda
>
^ permalink raw reply
* Re: [Qemu-devel] [PATCH] ceph/rbd block driver for qemu-kvm (v4)
From: Anthony Liguori @ 2010-10-07 21:55 UTC (permalink / raw)
To: Yehuda Sadeh Weinraub
Cc: Kevin Wolf, kvm, qemu-devel, ceph-devel, Christian Brunner
In-Reply-To: <AANLkTinzdiJZFxacD1BDCPWt44ChokNaNVU6_E=GtcTD@mail.gmail.com>
On 10/07/2010 04:49 PM, Yehuda Sadeh Weinraub wrote:
> On Thu, Oct 7, 2010 at 2:04 PM, Anthony Liguori<anthony@codemonkey.ws> wrote:
>
>> On 10/07/2010 03:47 PM, Yehuda Sadeh Weinraub wrote:
>>
>>>> How is that possible? Are the callbacks delivered in the context of a
>>>> different thread? If so, don't you need locking?
>>>>
>>>>
>>> Not sure I'm completely following you. The callbacks are delivered in
>>> the context of a different thread, but won't run concurrently.
>>>
>> Concurrently to what? How do you prevent them from running concurrently
>> with qemu?
>>
> There are two types of callbacks. The first is for rados aio
> completions, and the second one is the one added later for the fd glue
> layer.
>
This is a bad architecture for something like qemu. You could create a
pipe and use the pipe to signal to qemu. Same principle as eventfd.
Ideally, you would do this in the library itself.
Regards,
Anthony Liguori
> The first callback, called by librados whenever aio completes, runs in
> the context of a single librados thread:
>
> +static void rbd_finish_aiocb(rados_completion_t c, RADOSCB *rcb)
> +{
> + RBDAIOCB *acb = rcb->acb;
> rcb is per a single aio. Was created before and will be destroyed
> here, whereas acb is shared between a few aios, however, it was
> generated before the first aio was created.
>
> + int64_t r;
> + uint64_t buf = 1;
> + int i;
> +
> + acb->aiocnt--;
>
> acb->aiocnt has been set before initiating all the aios, so it's ok to
> touch it now. Same goes to all acb fields.
>
> + r = rados_aio_get_return_value(c);
> + rados_aio_release(c);
> + if (acb->write) {
> + if (r< 0) {
> + acb->ret = r;
> + acb->error = 1;
> + } else if (!acb->error) {
> + acb->ret += rcb->segsize;
> + }
> + } else {
> + if (r == -ENOENT) {
> + memset(rcb->buf, 0, rcb->segsize);
> + if (!acb->error) {
> + acb->ret += rcb->segsize;
> + }
> + } else if (r< 0) {
> + acb->ret = r;
> + acb->error = 1;
> + } else if (r< rcb->segsize) {
> + memset(rcb->buf + r, 0, rcb->segsize - r);
> + if (!acb->error) {
> + acb->ret += rcb->segsize;
> + }
> + } else if (!acb->error) {
> + acb->ret += r;
> + }
> + }
> + if (write(acb->s->efd,&buf, sizeof(buf))< 0)
> This will wake up the io_read()
>
> + error_report("failed writing to acb->s->efd\n");
> + qemu_free(rcb);
> + i = 0;
> + if (!acb->aiocnt&& acb->bh) {
> + qemu_bh_schedule(acb->bh);
> This is the only qemu related call in here, seems safe to call it.
>
> + }
> +}
>
> The scheduled bh function will be called only after all aios that
> relate to this specific aio set are done, so the following seems ok,
> as there's no more acb references.
> +static void rbd_aio_bh_cb(void *opaque)
> +{
> + RBDAIOCB *acb = opaque;
> + uint64_t buf = 1;
> +
> + if (!acb->write) {
> + qemu_iovec_from_buffer(acb->qiov, acb->bounce, acb->qiov->size);
> + }
> + qemu_vfree(acb->bounce);
> + acb->common.cb(acb->common.opaque, (acb->ret> 0 ? 0 : acb->ret));
> + qemu_bh_delete(acb->bh);
> + acb->bh = NULL;
> +
> + if (write(acb->s->efd,&buf, sizeof(buf))< 0)
> + error_report("failed writing to acb->s->efd\n");
> + qemu_aio_release(acb);
> +}
>
> Now, the second ones are the io_read(), in which we have our glue fd.
> We send uint64 per each completed io
>
> +static void rbd_aio_completion_cb(void *opaque)
> +{
> + BDRVRBDState *s = opaque;
> +
> + uint64_t val;
> + ssize_t ret;
> +
> + do {
> + if ((ret = read(s->efd,&val, sizeof(val)))> 0) {
> + s->qemu_aio_count -= val;
> There is an issue here with s->qemu_aio_count which needs to be
> protected by a mutex. Other than that, it just reads from s->efd.
>
> + }
> + } while (ret< 0&& errno == EINTR);
> +
> + return;
> +}
> +
> +static int rbd_aio_flush_cb(void *opaque)
> +{
> + BDRVRBDState *s = opaque;
> +
> + return (s->qemu_aio_count> 0);
> Same here as with the previous one, needs a mutex around s->qemu_aio_count.
>
> +}
>
>
>> If you saw lock ups, I bet that's what it was from.
>>
>>
> As I explained before, before introducing the fd glue layer, the lack
> of fd associated with our block device caused that there was no way
> for qemu to check whether all aios were flushed or not, which didn't
> work well when doing migration/savevm.
>
> Thanks,
> Yehuda
>
^ permalink raw reply
* [PATCH] power: introduce library for device-specific OPPs
From: Rafael J. Wysocki @ 2010-10-07 21:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1286357180-21565-1-git-send-email-nm@ti.com>
Hi,
On Wednesday, October 06, 2010, Nishanth Menon wrote:
> SoCs have a standard set of tuples consisting of frequency and
> voltage pairs that the device will support per voltage domain. These
> are called Operating Performance Points or OPPs. The actual
> definitions of OPP varies over silicon versions. For a specific domain,
> we can have a set of {frequency, voltage} pairs. As the kernel boots
> and more information is available, a default set of these are activated
> based on the precise nature of device. Further on operation, based on
> conditions prevailing in the system (such as temperature), some OPP
> availability may be temporarily controlled by the SoC frameworks.
>
> To implement an OPP, some sort of power management support is necessary
> hence this library depends on CONFIG_PM.
The patch generally looks good to me, I only have a couple of cosmetic remarks
(below).
...
> +static int opp_set_availability(struct device *dev, unsigned long freq,
> + bool availability_req)
> +{
> + struct device_opp *tmp_dev_opp, *dev_opp = NULL;
> + struct opp *new_opp, *tmp_opp, *opp = ERR_PTR(-ENODEV);
> + int r = 0;
> +
> + /* keep the node allocated */
> + new_opp = kmalloc(sizeof(struct opp), GFP_KERNEL);
> + if (!new_opp) {
> + pr_warning("Unable to allocate opp\n");
Please add an identification string to the messages, something like
"OPP: Unable to allocat object\n" (and similarly in the other messages).
That would help to find the source of a message in case there's any problem.
> + return -ENOMEM;
> + }
> +
> + mutex_lock(&dev_opp_list_lock);
> +
> + /* Find the device_opp */
> + list_for_each_entry(tmp_dev_opp, &dev_opp_list, node) {
> + if (dev == tmp_dev_opp->dev) {
> + dev_opp = tmp_dev_opp;
> + break;
> + }
> + }
> + if (IS_ERR(dev_opp)) {
> + r = PTR_ERR(dev_opp);
> + pr_warning("Unable to find device\n");
> + goto out1;
I'd prefer this lable to be called "unlock". It will be a bit more informative.
> + }
> +
> + /* Do we have the frequency? */
> + list_for_each_entry(tmp_opp, &dev_opp->opp_list, node) {
> + if (tmp_opp->rate == freq) {
> + opp = tmp_opp;
> + break;
> + }
> + }
> + if (IS_ERR(opp)) {
> + r = PTR_ERR(opp);
> + goto out1;
> + }
> +
> + /* Is update really needed? */
> + if (opp->available == availability_req)
> + goto out1;
> + /* copy the old data over */
> + *new_opp = *opp;
> +
> + /* plug in new node */
> + new_opp->available = availability_req;
> +
> + list_replace_rcu(&opp->node, &new_opp->node);
> + mutex_unlock(&dev_opp_list_lock);
> + synchronize_rcu();
> +
> + /* clean up old opp */
> + new_opp = opp;
> + goto out;
> +
> +out1:
+unlock:
> + mutex_unlock(&dev_opp_list_lock);
> +out:
> + kfree(new_opp);
> + return r;
> +}
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH] power: introduce library for device-specific OPPs
From: Rafael J. Wysocki @ 2010-10-07 21:54 UTC (permalink / raw)
To: Nishanth Menon; +Cc: Paul, linux-pm, l-o, lkml, l-a
In-Reply-To: <1286357180-21565-1-git-send-email-nm@ti.com>
Hi,
On Wednesday, October 06, 2010, Nishanth Menon wrote:
> SoCs have a standard set of tuples consisting of frequency and
> voltage pairs that the device will support per voltage domain. These
> are called Operating Performance Points or OPPs. The actual
> definitions of OPP varies over silicon versions. For a specific domain,
> we can have a set of {frequency, voltage} pairs. As the kernel boots
> and more information is available, a default set of these are activated
> based on the precise nature of device. Further on operation, based on
> conditions prevailing in the system (such as temperature), some OPP
> availability may be temporarily controlled by the SoC frameworks.
>
> To implement an OPP, some sort of power management support is necessary
> hence this library depends on CONFIG_PM.
The patch generally looks good to me, I only have a couple of cosmetic remarks
(below).
...
> +static int opp_set_availability(struct device *dev, unsigned long freq,
> + bool availability_req)
> +{
> + struct device_opp *tmp_dev_opp, *dev_opp = NULL;
> + struct opp *new_opp, *tmp_opp, *opp = ERR_PTR(-ENODEV);
> + int r = 0;
> +
> + /* keep the node allocated */
> + new_opp = kmalloc(sizeof(struct opp), GFP_KERNEL);
> + if (!new_opp) {
> + pr_warning("Unable to allocate opp\n");
Please add an identification string to the messages, something like
"OPP: Unable to allocat object\n" (and similarly in the other messages).
That would help to find the source of a message in case there's any problem.
> + return -ENOMEM;
> + }
> +
> + mutex_lock(&dev_opp_list_lock);
> +
> + /* Find the device_opp */
> + list_for_each_entry(tmp_dev_opp, &dev_opp_list, node) {
> + if (dev == tmp_dev_opp->dev) {
> + dev_opp = tmp_dev_opp;
> + break;
> + }
> + }
> + if (IS_ERR(dev_opp)) {
> + r = PTR_ERR(dev_opp);
> + pr_warning("Unable to find device\n");
> + goto out1;
I'd prefer this lable to be called "unlock". It will be a bit more informative.
> + }
> +
> + /* Do we have the frequency? */
> + list_for_each_entry(tmp_opp, &dev_opp->opp_list, node) {
> + if (tmp_opp->rate == freq) {
> + opp = tmp_opp;
> + break;
> + }
> + }
> + if (IS_ERR(opp)) {
> + r = PTR_ERR(opp);
> + goto out1;
> + }
> +
> + /* Is update really needed? */
> + if (opp->available == availability_req)
> + goto out1;
> + /* copy the old data over */
> + *new_opp = *opp;
> +
> + /* plug in new node */
> + new_opp->available = availability_req;
> +
> + list_replace_rcu(&opp->node, &new_opp->node);
> + mutex_unlock(&dev_opp_list_lock);
> + synchronize_rcu();
> +
> + /* clean up old opp */
> + new_opp = opp;
> + goto out;
> +
> +out1:
+unlock:
> + mutex_unlock(&dev_opp_list_lock);
> +out:
> + kfree(new_opp);
> + return r;
> +}
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH] power: introduce library for device-specific OPPs
From: Rafael J. Wysocki @ 2010-10-07 21:54 UTC (permalink / raw)
To: Nishanth Menon; +Cc: linux-pm, lkml, l-o, l-a, Paul
In-Reply-To: <1286357180-21565-1-git-send-email-nm@ti.com>
Hi,
On Wednesday, October 06, 2010, Nishanth Menon wrote:
> SoCs have a standard set of tuples consisting of frequency and
> voltage pairs that the device will support per voltage domain. These
> are called Operating Performance Points or OPPs. The actual
> definitions of OPP varies over silicon versions. For a specific domain,
> we can have a set of {frequency, voltage} pairs. As the kernel boots
> and more information is available, a default set of these are activated
> based on the precise nature of device. Further on operation, based on
> conditions prevailing in the system (such as temperature), some OPP
> availability may be temporarily controlled by the SoC frameworks.
>
> To implement an OPP, some sort of power management support is necessary
> hence this library depends on CONFIG_PM.
The patch generally looks good to me, I only have a couple of cosmetic remarks
(below).
...
> +static int opp_set_availability(struct device *dev, unsigned long freq,
> + bool availability_req)
> +{
> + struct device_opp *tmp_dev_opp, *dev_opp = NULL;
> + struct opp *new_opp, *tmp_opp, *opp = ERR_PTR(-ENODEV);
> + int r = 0;
> +
> + /* keep the node allocated */
> + new_opp = kmalloc(sizeof(struct opp), GFP_KERNEL);
> + if (!new_opp) {
> + pr_warning("Unable to allocate opp\n");
Please add an identification string to the messages, something like
"OPP: Unable to allocat object\n" (and similarly in the other messages).
That would help to find the source of a message in case there's any problem.
> + return -ENOMEM;
> + }
> +
> + mutex_lock(&dev_opp_list_lock);
> +
> + /* Find the device_opp */
> + list_for_each_entry(tmp_dev_opp, &dev_opp_list, node) {
> + if (dev == tmp_dev_opp->dev) {
> + dev_opp = tmp_dev_opp;
> + break;
> + }
> + }
> + if (IS_ERR(dev_opp)) {
> + r = PTR_ERR(dev_opp);
> + pr_warning("Unable to find device\n");
> + goto out1;
I'd prefer this lable to be called "unlock". It will be a bit more informative.
> + }
> +
> + /* Do we have the frequency? */
> + list_for_each_entry(tmp_opp, &dev_opp->opp_list, node) {
> + if (tmp_opp->rate == freq) {
> + opp = tmp_opp;
> + break;
> + }
> + }
> + if (IS_ERR(opp)) {
> + r = PTR_ERR(opp);
> + goto out1;
> + }
> +
> + /* Is update really needed? */
> + if (opp->available == availability_req)
> + goto out1;
> + /* copy the old data over */
> + *new_opp = *opp;
> +
> + /* plug in new node */
> + new_opp->available = availability_req;
> +
> + list_replace_rcu(&opp->node, &new_opp->node);
> + mutex_unlock(&dev_opp_list_lock);
> + synchronize_rcu();
> +
> + /* clean up old opp */
> + new_opp = opp;
> + goto out;
> +
> +out1:
+unlock:
> + mutex_unlock(&dev_opp_list_lock);
> +out:
> + kfree(new_opp);
> + return r;
> +}
Thanks,
Rafael
^ permalink raw reply
* RE: [PATCH 1/2] drivers:bluetooth: TI_ST bluetooth driver
From: Savoy, Pavan @ 2010-10-07 21:53 UTC (permalink / raw)
To: Gustavo F. Padovan
Cc: linux-bluetooth@vger.kernel.org, marcel@holtmann.org,
greg@kroah.com, linux-kernel@vger.kernel.org
In-Reply-To: <20101007184534.GB13602@vigoh>
Gustavo / Marcel,
> -----Original Message-----
> From: Gustavo F. Padovan [mailto:pao@profusion.mobi] On Behalf Of Gustavo F.
> Padovan
> Sent: Thursday, October 07, 2010 1:46 PM
> To: Savoy, Pavan
> Cc: linux-bluetooth@vger.kernel.org; marcel@holtmann.org; greg@kroah.com;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/2] drivers:bluetooth: TI_ST bluetooth driver
>
> Hi Pavan,
>
> Change the commit subject to "Bluetooth: TI_ST bluetooth driver"
>
> * pavan_savoy@ti.com <pavan_savoy@ti.com> [2010-10-07 14:47:16 -0400]:
>
> > From: Pavan Savoy <pavan_savoy@ti.com>
> >
> > This is the bluetooth protocol driver for the TI WiLink7 chipsets.
> > Texas Instrument's WiLink chipsets combine wireless technologies
> > like BT, FM, GPS and WLAN onto a single chip.
> >
> > This Bluetooth driver works on top of the TI_ST shared transport
> > line discipline driver which also allows other drivers like
> > FM V4L2 and GPS character driver to make use of the same UART interface.
> >
> > Signed-off-by: Pavan Savoy <pavan_savoy@ti.com>
> > ---
> > drivers/bluetooth/bt_ti.c | 489
> +++++++++++++++++++++++++++++++++++++++++++++
> > 1 files changed, 489 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/bluetooth/bt_ti.c
>
> We don't have filename with bt_.. in drivers/bluetooth/. Maybe ti_st.c
> should be a better name, or something like that.
>
> >
> > diff --git a/drivers/bluetooth/bt_ti.c b/drivers/bluetooth/bt_ti.c
> > new file mode 100644
> > index 0000000..dffbb56
> > --- /dev/null
> > +++ b/drivers/bluetooth/bt_ti.c
> > @@ -0,0 +1,489 @@
> > +/*
> > + * Texas Instrument's Bluetooth Driver For Shared Transport.
> > + *
> > + * Bluetooth Driver acts as interface between HCI CORE and
> > + * TI Shared Transport Layer.
> > + *
> > + * Copyright (C) 2009-2010 Texas Instruments
> > + * Author: Raja Mani <raja_mani@ti.com>
> > + * Pavan Savoy <pavan_savoy@ti.com>
> > + *
> > + * 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.
> > + *
> > + * 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, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
> USA
> > + *
> > + */
> > +
> > +#include <linux/platform_device.h>
> > +#include <net/bluetooth/bluetooth.h>
> > +#include <net/bluetooth/hci_core.h>
> > +
> > +#include <linux/ti_wilink_st.h>
> > +
> > +/* Bluetooth Driver Version */
> > +#define VERSION "1.0"
> > +
> > +/* Defines number of seconds to wait for reg completion
> > + * callback getting called from ST (in case,registration
> > + * with ST returns PENDING status)
> > + */
> > +#define BT_REGISTER_TIMEOUT 6000 /* 6 sec */
> > +
> > +/* BT driver's local status */
> > +#define BT_DRV_RUNNING 0
> > +#define BT_ST_REGISTERED 1
> > +
> > +/**
> > + * struct hci_st - BT driver operation structure
> > + * @hdev: hci device pointer which binds to bt driver
> > + * @flags: used locally,to maintain various BT driver status
> > + * @streg_cbdata: to hold ST registration callback status
> > + * @st_write: write function pointer of ST driver
> > + * @wait_for_btdrv_reg_completion - completion sync between hci_st_open
> > + * and hci_st_registration_completion_cb.
> > + */
> > +struct hci_st {
> > + struct hci_dev *hdev;
> > + unsigned long flags;
> > + char streg_cbdata;
> > + long (*st_write) (struct sk_buff *);
> > + struct completion wait_for_btdrv_reg_completion;
> > +};
> > +
> > +static int reset;
> > +
> > +/* Increments HCI counters based on pocket ID (cmd,acl,sco) */
> > +static inline void hci_st_tx_complete(struct hci_st *hst, int pkt_type)
> > +{
> > + struct hci_dev *hdev;
> > + hdev = hst->hdev;
> > +
> > + /* Update HCI stat counters */
> > + switch (pkt_type) {
> > + case HCI_COMMAND_PKT:
> > + hdev->stat.cmd_tx++;
> > + break;
> > +
> > + case HCI_ACLDATA_PKT:
> > + hdev->stat.acl_tx++;
> > + break;
> > +
> > + case HCI_SCODATA_PKT:
> > + hdev->stat.cmd_tx++;
>
> it should be sco_tx here.
>
> > + break;
> > + }
> > +}
> > +
> > +/* ------- Interfaces to Shared Transport ------ */
> > +
> > +/* Called by ST layer to indicate protocol registration completion
> > + * status.hci_st_open() function will wait for signal from this
> > + * API when st_register() function returns ST_PENDING.
> > + */
> > +static void hci_st_registration_completion_cb(void *priv_data, char data)
>
> That is not the hci layer, so rename this function (and the others) to
> something that reflect where they are really doing.
>
> > +{
> > + struct hci_st *lhst = (struct hci_st *)priv_data;
> > + /* hci_st_open() function needs value of 'data' to know
> > + * the registration status(success/fail),So have a back
> > + * up of it.
> > + */
> > + lhst->streg_cbdata = data;
> > +
> > + /* Got a feedback from ST for BT driver registration
> > + * request.Wackup hci_st_open() function to continue
> > + * it's open operation.
> > + */
> > + complete(&lhst->wait_for_btdrv_reg_completion);
> > +}
> > +
> > +/* Called by Shared Transport layer when receive data is
> > + * available */
> > +static long hci_st_receive(void *priv_data, struct sk_buff *skb)
> > +{
> > + int err;
> > + int len;
>
> you can put err and len in the same line.
>
> > + struct hci_st *lhst = (struct hci_st *)priv_data;
> > +
> > + err = 0;
> > + len = 0;
>
> and no need to set them to 0 here.
>
> > +
> > + if (skb == NULL) {
> > + BT_ERR("Invalid SKB received from ST");
> > + return -EFAULT;
> > + }
>
> We need a empty line here.
>
> > + if (!lhst) {
> > + kfree_skb(skb);
> > + BT_ERR("Invalid hci_st memory,freeing SKB");
> > + return -EFAULT;
> > + }
>
> And also here. Check the rest of the code for similar issues.
>
> > + if (!test_bit(BT_DRV_RUNNING, &lhst->flags)) {
> > + kfree_skb(skb);
> > + BT_ERR("Device is not running,freeing SKB");
> > + return -EINVAL;
> > + }
>
> If you are here, your device is running, right? Or am I missing
> something?
>
> > +
> > + len = skb->len;
> > + skb->dev = (struct net_device *)lhst->hdev;
> > +
> > + /* Forward skb to HCI CORE layer */
> > + err = hci_recv_frame(skb);
> > + if (err) {
> > + kfree_skb(skb);
> > + BT_ERR("Unable to push skb to HCI CORE(%d),freeing SKB",
> > + err);
> > + return err;
> > + }
> > + lhst->hdev->stat.byte_rx += len;
>
> actually you even don't need len, just use skb->len
>
> > +
> > + return 0;
> > +}
> > +
> > +/* ------- Interfaces to HCI layer ------ */
> > +
> > +/* Called from HCI core to initialize the device */
> > +static int hci_st_open(struct hci_dev *hdev)
> > +{
> > + static struct st_proto_s hci_st_proto;
> > + unsigned long timeleft;
> > + struct hci_st *hst;
> > + int err;
> > + err = 0;
> > +
> > + BT_DBG("%s %p", hdev->name, hdev);
> > + hst = hdev->driver_data;
> > +
> > + /* Populate BT driver info required by ST */
> > + memset(&hci_st_proto, 0, sizeof(hci_st_proto));
> > +
> > + /* BT driver ID */
> > + hci_st_proto.type = ST_BT;
> > +
> > + /* Receive function which called from ST */
> > + hci_st_proto.recv = hci_st_receive;
> > +
> > + /* Packet match function may used in future */
> > + hci_st_proto.match_packet = NULL;
>
> It is already NULL, you dua a memset.
>
> > +
> > + /* Callback to be called when registration is pending */
> > + hci_st_proto.reg_complete_cb = hci_st_registration_completion_cb;
> > +
> > + /* This is write function pointer of ST. BT driver will make use of
> this
> > + * for sending any packets to chip. ST will assign and give to us, so
> > + * make it as NULL */
> > + hci_st_proto.write = NULL;
>
> Same here.
>
> > +
> > + /* send in the hst to be received at registration complete callback
> > + * and during st's receive
> > + */
> > + hci_st_proto.priv_data = hst;
> > +
> > + /* Register with ST layer */
> > + err = st_register(&hci_st_proto);
> > + if (err == -EINPROGRESS) {
> > + /* Prepare wait-for-completion handler data structures.
> > + * Needed to syncronize this and st_registration_completion_cb()
> > + * functions.
> > + */
> > + init_completion(&hst->wait_for_btdrv_reg_completion);
>
> I'm not liking that, but I'll leave for Marcel and others comment.
Thanks for the comments, I will take care of them all, but I want to
tell why I do this here.
"st_register" function takes time. It takes about 2 seconds for us
to download the firmware. (or longer based on UART baud-rate).
This is intended to happen in the process context. Say hciconfig hci0 up
takes a while longer than usual or if someone doing an ioctl(HCIDEVUP)
and this is also the reason I have a callback, as and when firmware download
completes, I call the Bluetooth driver's callback.
and I await marcel's comment on this.
> > +
> > + /* Reset ST registration callback status flag , this value
> > + * will be updated in hci_st_registration_completion_cb()
> > + * function whenever it called from ST driver.
> > + */
> > + hst->streg_cbdata = -EINPROGRESS;
> > +
> > + /* ST is busy with other protocol registration(may be busy with
> > + * firmware download).So,Wait till the registration callback
> > + * (passed as a argument to st_register() function) getting
> > + * called from ST.
> > + */
> > + BT_DBG(" %s waiting for reg completion signal from ST",
> > + __func__);
> > +
> > + timeleft =
> > + wait_for_completion_timeout
> > + (&hst->wait_for_btdrv_reg_completion,
> > + msecs_to_jiffies(BT_REGISTER_TIMEOUT));
> > + if (!timeleft) {
> > + BT_ERR("Timeout(%d sec),didn't get reg"
> > + "completion signal from ST",
> > + BT_REGISTER_TIMEOUT / 1000);
> > + return -ETIMEDOUT;
> > + }
> > +
> > + /* Is ST registration callback called with ERROR value? */
> > + if (hst->streg_cbdata != 0) {
> > + BT_ERR("ST reg completion CB called with invalid"
> > + "status %d", hst->streg_cbdata);
> > + return -EAGAIN;
> > + }
> > + err = 0;
> > + } else if (err == -1) {
>
> Use the proper error macro instead "-1"
>
> > + BT_ERR("st_register failed %d", err);
> > + return -EAGAIN;
> > + }
> > +
> > + /* Do we have proper ST write function? */
> > + if (hci_st_proto.write != NULL) {
> > + /* We need this pointer for sending any Bluetooth pkts */
> > + hst->st_write = hci_st_proto.write;
> > + } else {
> > + BT_ERR("failed to get ST write func pointer");
> > +
> > + /* Undo registration with ST */
> > + err = st_unregister(ST_BT);
> > + if (err < 0)
> > + BT_ERR("st_unregister failed %d", err);
> > +
> > + hst->st_write = NULL;
> > + return -EAGAIN;
> > + }
> > +
> > + /* Registration with ST layer is completed successfully,
> > + * now chip is ready to accept commands from HCI CORE.
> > + * Mark HCI Device flag as RUNNING
> > + */
> > + set_bit(HCI_RUNNING, &hdev->flags);
> > +
> > + /* Registration with ST successful */
> > + set_bit(BT_ST_REGISTERED, &hst->flags);
> > +
> > + return err;
> > +}
> > +
> > +/* Close device */
> > +static int hci_st_close(struct hci_dev *hdev)
> > +{
> > + int err;
> > + struct hci_st *hst;
>
> Skip a line after declarations.
>
> > + err = 0;
>
> you can set err to 0 in the declaration if you really need that.
>
> > +
> > + hst = hdev->driver_data;
> > + /* Unregister from ST layer */
> > + if (test_and_clear_bit(BT_ST_REGISTERED, &hst->flags)) {
> > + err = st_unregister(ST_BT);
> > + if (err != 0) {
> > + BT_ERR("st_unregister failed %d", err);
> > + return -EBUSY;
> > + }
> > + }
> > +
> > + hst->st_write = NULL;
> > +
> > + /* ST layer would have moved chip to inactive state.
> > + * So,clear HCI device RUNNING flag.
> > + */
> > + if (!test_and_clear_bit(HCI_RUNNING, &hdev->flags))
> > + return 0;
>
> Looks you are screwing up the flags here, if it fails on st_unregister()
> and returns HCI_RUNNING should keep set?
>
> > +
> > + return err;
>
> Rethink how you are doing error handling here, it should no be
> complicated like that.
>
> > +}
> > +
> > +/* Called from HCI CORE , Sends frames to Shared Transport */
> > +static int hci_st_send_frame(struct sk_buff *skb)
> > +{
> > + struct hci_dev *hdev;
> > + struct hci_st *hst;
> > + long len;
> > +
> > + if (skb == NULL) {
> > + BT_ERR("Invalid skb received from HCI CORE");
> > + return -ENOMEM;
> > + }
> > + hdev = (struct hci_dev *)skb->dev;
> > + if (!hdev) {
> > + BT_ERR("SKB received for invalid HCI Device (hdev=NULL)");
> > + return -ENODEV;
> > + }
> > + if (!test_bit(HCI_RUNNING, &hdev->flags)) {
> > + BT_ERR("Device is not running");
> > + return -EBUSY;
> > + }
> > +
> > + hst = (struct hci_st *)hdev->driver_data;
> > +
> > + /* Prepend skb with frame type */
> > + memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
> > +
> > + BT_DBG(" %s: type %d len %d", hdev->name, bt_cb(skb)->pkt_type,
> > + skb->len);
> > +
> > + /* Insert skb to shared transport layer's transmit queue.
> > + * Freeing skb memory is taken care in shared transport layer,
> > + * so don't free skb memory here.
> > + */
> > + if (!hst->st_write) {
> > + kfree_skb(skb);
> > + BT_ERR(" Can't write to ST, st_write null?");
> > + return -EAGAIN;
> > + }
> > + len = hst->st_write(skb);
> > + if (len < 0) {
> > + /* Something went wrong in st write , free skb memory */
>
> IMHO we don't need comments like that, clearly we now that something
> went wrong.
>
> > + kfree_skb(skb);
> > + BT_ERR(" ST write failed (%ld)", len);
> > + return -EAGAIN;
> > + }
> > +
> > + /* ST accepted our skb. So, Go ahead and do rest */
> > + hdev->stat.byte_tx += len;
> > + hci_st_tx_complete(hst, bt_cb(skb)->pkt_type);
> > +
> > + return 0;
>
> goto might be better to handle error here.
>
>
> --
> Gustavo F. Padovan
> ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* RE: [PATCH 1/2] drivers:bluetooth: TI_ST bluetooth driver
From: Savoy, Pavan @ 2010-10-07 21:53 UTC (permalink / raw)
To: Gustavo F. Padovan
Cc: linux-bluetooth@vger.kernel.org, marcel@holtmann.org,
greg@kroah.com, linux-kernel@vger.kernel.org
In-Reply-To: <20101007184534.GB13602@vigoh>
Gustavo / Marcel,
=20
> -----Original Message-----
> From: Gustavo F. Padovan [mailto:pao@profusion.mobi] On Behalf Of Gustavo=
F.
> Padovan
> Sent: Thursday, October 07, 2010 1:46 PM
> To: Savoy, Pavan
> Cc: linux-bluetooth@vger.kernel.org; marcel@holtmann.org; greg@kroah.com;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/2] drivers:bluetooth: TI_ST bluetooth driver
>=20
> Hi Pavan,
>=20
> Change the commit subject to "Bluetooth: TI_ST bluetooth driver"
>=20
> * pavan_savoy@ti.com <pavan_savoy@ti.com> [2010-10-07 14:47:16 -0400]:
>=20
> > From: Pavan Savoy <pavan_savoy@ti.com>
> >
> > This is the bluetooth protocol driver for the TI WiLink7 chipsets.
> > Texas Instrument's WiLink chipsets combine wireless technologies
> > like BT, FM, GPS and WLAN onto a single chip.
> >
> > This Bluetooth driver works on top of the TI_ST shared transport
> > line discipline driver which also allows other drivers like
> > FM V4L2 and GPS character driver to make use of the same UART interface=
.
> >
> > Signed-off-by: Pavan Savoy <pavan_savoy@ti.com>
> > ---
> > drivers/bluetooth/bt_ti.c | 489
> +++++++++++++++++++++++++++++++++++++++++++++
> > 1 files changed, 489 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/bluetooth/bt_ti.c
>=20
> We don't have filename with bt_.. in drivers/bluetooth/. Maybe ti_st.c
> should be a better name, or something like that.
>=20
> >
> > diff --git a/drivers/bluetooth/bt_ti.c b/drivers/bluetooth/bt_ti.c
> > new file mode 100644
> > index 0000000..dffbb56
> > --- /dev/null
> > +++ b/drivers/bluetooth/bt_ti.c
> > @@ -0,0 +1,489 @@
> > +/*
> > + * Texas Instrument's Bluetooth Driver For Shared Transport.
> > + *
> > + * Bluetooth Driver acts as interface between HCI CORE and
> > + * TI Shared Transport Layer.
> > + *
> > + * Copyright (C) 2009-2010 Texas Instruments
> > + * Author: Raja Mani <raja_mani@ti.com>
> > + * Pavan Savoy <pavan_savoy@ti.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modi=
fy
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * 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, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-13=
07
> USA
> > + *
> > + */
> > +
> > +#include <linux/platform_device.h>
> > +#include <net/bluetooth/bluetooth.h>
> > +#include <net/bluetooth/hci_core.h>
> > +
> > +#include <linux/ti_wilink_st.h>
> > +
> > +/* Bluetooth Driver Version */
> > +#define VERSION "1.0"
> > +
> > +/* Defines number of seconds to wait for reg completion
> > + * callback getting called from ST (in case,registration
> > + * with ST returns PENDING status)
> > + */
> > +#define BT_REGISTER_TIMEOUT 6000 /* 6 sec */
> > +
> > +/* BT driver's local status */
> > +#define BT_DRV_RUNNING 0
> > +#define BT_ST_REGISTERED 1
> > +
> > +/**
> > + * struct hci_st - BT driver operation structure
> > + * @hdev: hci device pointer which binds to bt driver
> > + * @flags: used locally,to maintain various BT driver status
> > + * @streg_cbdata: to hold ST registration callback status
> > + * @st_write: write function pointer of ST driver
> > + * @wait_for_btdrv_reg_completion - completion sync between hci_st_ope=
n
> > + * and hci_st_registration_completion_cb.
> > + */
> > +struct hci_st {
> > + struct hci_dev *hdev;
> > + unsigned long flags;
> > + char streg_cbdata;
> > + long (*st_write) (struct sk_buff *);
> > + struct completion wait_for_btdrv_reg_completion;
> > +};
> > +
> > +static int reset;
> > +
> > +/* Increments HCI counters based on pocket ID (cmd,acl,sco) */
> > +static inline void hci_st_tx_complete(struct hci_st *hst, int pkt_type=
)
> > +{
> > + struct hci_dev *hdev;
> > + hdev =3D hst->hdev;
> > +
> > + /* Update HCI stat counters */
> > + switch (pkt_type) {
> > + case HCI_COMMAND_PKT:
> > + hdev->stat.cmd_tx++;
> > + break;
> > +
> > + case HCI_ACLDATA_PKT:
> > + hdev->stat.acl_tx++;
> > + break;
> > +
> > + case HCI_SCODATA_PKT:
> > + hdev->stat.cmd_tx++;
>=20
> it should be sco_tx here.
>=20
> > + break;
> > + }
> > +}
> > +
> > +/* ------- Interfaces to Shared Transport ------ */
> > +
> > +/* Called by ST layer to indicate protocol registration completion
> > + * status.hci_st_open() function will wait for signal from this
> > + * API when st_register() function returns ST_PENDING.
> > + */
> > +static void hci_st_registration_completion_cb(void *priv_data, char da=
ta)
>=20
> That is not the hci layer, so rename this function (and the others) to
> something that reflect where they are really doing.
>=20
> > +{
> > + struct hci_st *lhst =3D (struct hci_st *)priv_data;
> > + /* hci_st_open() function needs value of 'data' to know
> > + * the registration status(success/fail),So have a back
> > + * up of it.
> > + */
> > + lhst->streg_cbdata =3D data;
> > +
> > + /* Got a feedback from ST for BT driver registration
> > + * request.Wackup hci_st_open() function to continue
> > + * it's open operation.
> > + */
> > + complete(&lhst->wait_for_btdrv_reg_completion);
> > +}
> > +
> > +/* Called by Shared Transport layer when receive data is
> > + * available */
> > +static long hci_st_receive(void *priv_data, struct sk_buff *skb)
> > +{
> > + int err;
> > + int len;
>=20
> you can put err and len in the same line.
>=20
> > + struct hci_st *lhst =3D (struct hci_st *)priv_data;
> > +
> > + err =3D 0;
> > + len =3D 0;
>=20
> and no need to set them to 0 here.
>=20
> > +
> > + if (skb =3D=3D NULL) {
> > + BT_ERR("Invalid SKB received from ST");
> > + return -EFAULT;
> > + }
>=20
> We need a empty line here.
>=20
> > + if (!lhst) {
> > + kfree_skb(skb);
> > + BT_ERR("Invalid hci_st memory,freeing SKB");
> > + return -EFAULT;
> > + }
>=20
> And also here. Check the rest of the code for similar issues.
>=20
> > + if (!test_bit(BT_DRV_RUNNING, &lhst->flags)) {
> > + kfree_skb(skb);
> > + BT_ERR("Device is not running,freeing SKB");
> > + return -EINVAL;
> > + }
>=20
> If you are here, your device is running, right? Or am I missing
> something?
>=20
> > +
> > + len =3D skb->len;
> > + skb->dev =3D (struct net_device *)lhst->hdev;
> > +
> > + /* Forward skb to HCI CORE layer */
> > + err =3D hci_recv_frame(skb);
> > + if (err) {
> > + kfree_skb(skb);
> > + BT_ERR("Unable to push skb to HCI CORE(%d),freeing SKB",
> > + err);
> > + return err;
> > + }
> > + lhst->hdev->stat.byte_rx +=3D len;
>=20
> actually you even don't need len, just use skb->len
>=20
> > +
> > + return 0;
> > +}
> > +
> > +/* ------- Interfaces to HCI layer ------ */
> > +
> > +/* Called from HCI core to initialize the device */
> > +static int hci_st_open(struct hci_dev *hdev)
> > +{
> > + static struct st_proto_s hci_st_proto;
> > + unsigned long timeleft;
> > + struct hci_st *hst;
> > + int err;
> > + err =3D 0;
> > +
> > + BT_DBG("%s %p", hdev->name, hdev);
> > + hst =3D hdev->driver_data;
> > +
> > + /* Populate BT driver info required by ST */
> > + memset(&hci_st_proto, 0, sizeof(hci_st_proto));
> > +
> > + /* BT driver ID */
> > + hci_st_proto.type =3D ST_BT;
> > +
> > + /* Receive function which called from ST */
> > + hci_st_proto.recv =3D hci_st_receive;
> > +
> > + /* Packet match function may used in future */
> > + hci_st_proto.match_packet =3D NULL;
>=20
> It is already NULL, you dua a memset.
>=20
> > +
> > + /* Callback to be called when registration is pending */
> > + hci_st_proto.reg_complete_cb =3D hci_st_registration_completion_cb;
> > +
> > + /* This is write function pointer of ST. BT driver will make use of
> this
> > + * for sending any packets to chip. ST will assign and give to us, so
> > + * make it as NULL */
> > + hci_st_proto.write =3D NULL;
>=20
> Same here.
>=20
> > +
> > + /* send in the hst to be received at registration complete callback
> > + * and during st's receive
> > + */
> > + hci_st_proto.priv_data =3D hst;
> > +
> > + /* Register with ST layer */
> > + err =3D st_register(&hci_st_proto);
> > + if (err =3D=3D -EINPROGRESS) {
> > + /* Prepare wait-for-completion handler data structures.
> > + * Needed to syncronize this and st_registration_completion_cb()
> > + * functions.
> > + */
> > + init_completion(&hst->wait_for_btdrv_reg_completion);
>=20
> I'm not liking that, but I'll leave for Marcel and others comment.
Thanks for the comments, I will take care of them all, but I want to
tell why I do this here.
"st_register" function takes time. It takes about 2 seconds for us
to download the firmware. (or longer based on UART baud-rate).
This is intended to happen in the process context. Say hciconfig hci0 up
takes a while longer than usual or if someone doing an ioctl(HCIDEVUP)
and this is also the reason I have a callback, as and when firmware downloa=
d
completes, I call the Bluetooth driver's callback.
and I await marcel's comment on this.
> > +
> > + /* Reset ST registration callback status flag , this value
> > + * will be updated in hci_st_registration_completion_cb()
> > + * function whenever it called from ST driver.
> > + */
> > + hst->streg_cbdata =3D -EINPROGRESS;
> > +
> > + /* ST is busy with other protocol registration(may be busy with
> > + * firmware download).So,Wait till the registration callback
> > + * (passed as a argument to st_register() function) getting
> > + * called from ST.
> > + */
> > + BT_DBG(" %s waiting for reg completion signal from ST",
> > + __func__);
> > +
> > + timeleft =3D
> > + wait_for_completion_timeout
> > + (&hst->wait_for_btdrv_reg_completion,
> > + msecs_to_jiffies(BT_REGISTER_TIMEOUT));
> > + if (!timeleft) {
> > + BT_ERR("Timeout(%d sec),didn't get reg"
> > + "completion signal from ST",
> > + BT_REGISTER_TIMEOUT / 1000);
> > + return -ETIMEDOUT;
> > + }
> > +
> > + /* Is ST registration callback called with ERROR value? */
> > + if (hst->streg_cbdata !=3D 0) {
> > + BT_ERR("ST reg completion CB called with invalid"
> > + "status %d", hst->streg_cbdata);
> > + return -EAGAIN;
> > + }
> > + err =3D 0;
> > + } else if (err =3D=3D -1) {
>=20
> Use the proper error macro instead "-1"
>=20
> > + BT_ERR("st_register failed %d", err);
> > + return -EAGAIN;
> > + }
> > +
> > + /* Do we have proper ST write function? */
> > + if (hci_st_proto.write !=3D NULL) {
> > + /* We need this pointer for sending any Bluetooth pkts */
> > + hst->st_write =3D hci_st_proto.write;
> > + } else {
> > + BT_ERR("failed to get ST write func pointer");
> > +
> > + /* Undo registration with ST */
> > + err =3D st_unregister(ST_BT);
> > + if (err < 0)
> > + BT_ERR("st_unregister failed %d", err);
> > +
> > + hst->st_write =3D NULL;
> > + return -EAGAIN;
> > + }
> > +
> > + /* Registration with ST layer is completed successfully,
> > + * now chip is ready to accept commands from HCI CORE.
> > + * Mark HCI Device flag as RUNNING
> > + */
> > + set_bit(HCI_RUNNING, &hdev->flags);
> > +
> > + /* Registration with ST successful */
> > + set_bit(BT_ST_REGISTERED, &hst->flags);
> > +
> > + return err;
> > +}
> > +
> > +/* Close device */
> > +static int hci_st_close(struct hci_dev *hdev)
> > +{
> > + int err;
> > + struct hci_st *hst;
>=20
> Skip a line after declarations.
>=20
> > + err =3D 0;
>=20
> you can set err to 0 in the declaration if you really need that.
>=20
> > +
> > + hst =3D hdev->driver_data;
> > + /* Unregister from ST layer */
> > + if (test_and_clear_bit(BT_ST_REGISTERED, &hst->flags)) {
> > + err =3D st_unregister(ST_BT);
> > + if (err !=3D 0) {
> > + BT_ERR("st_unregister failed %d", err);
> > + return -EBUSY;
> > + }
> > + }
> > +
> > + hst->st_write =3D NULL;
> > +
> > + /* ST layer would have moved chip to inactive state.
> > + * So,clear HCI device RUNNING flag.
> > + */
> > + if (!test_and_clear_bit(HCI_RUNNING, &hdev->flags))
> > + return 0;
>=20
> Looks you are screwing up the flags here, if it fails on st_unregister()
> and returns HCI_RUNNING should keep set?
>=20
> > +
> > + return err;
>=20
> Rethink how you are doing error handling here, it should no be
> complicated like that.
>=20
> > +}
> > +
> > +/* Called from HCI CORE , Sends frames to Shared Transport */
> > +static int hci_st_send_frame(struct sk_buff *skb)
> > +{
> > + struct hci_dev *hdev;
> > + struct hci_st *hst;
> > + long len;
> > +
> > + if (skb =3D=3D NULL) {
> > + BT_ERR("Invalid skb received from HCI CORE");
> > + return -ENOMEM;
> > + }
> > + hdev =3D (struct hci_dev *)skb->dev;
> > + if (!hdev) {
> > + BT_ERR("SKB received for invalid HCI Device (hdev=3DNULL)");
> > + return -ENODEV;
> > + }
> > + if (!test_bit(HCI_RUNNING, &hdev->flags)) {
> > + BT_ERR("Device is not running");
> > + return -EBUSY;
> > + }
> > +
> > + hst =3D (struct hci_st *)hdev->driver_data;
> > +
> > + /* Prepend skb with frame type */
> > + memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
> > +
> > + BT_DBG(" %s: type %d len %d", hdev->name, bt_cb(skb)->pkt_type,
> > + skb->len);
> > +
> > + /* Insert skb to shared transport layer's transmit queue.
> > + * Freeing skb memory is taken care in shared transport layer,
> > + * so don't free skb memory here.
> > + */
> > + if (!hst->st_write) {
> > + kfree_skb(skb);
> > + BT_ERR(" Can't write to ST, st_write null?");
> > + return -EAGAIN;
> > + }
> > + len =3D hst->st_write(skb);
> > + if (len < 0) {
> > + /* Something went wrong in st write , free skb memory */
>=20
> IMHO we don't need comments like that, clearly we now that something
> went wrong.
>=20
> > + kfree_skb(skb);
> > + BT_ERR(" ST write failed (%ld)", len);
> > + return -EAGAIN;
> > + }
> > +
> > + /* ST accepted our skb. So, Go ahead and do rest */
> > + hdev->stat.byte_tx +=3D len;
> > + hci_st_tx_complete(hst, bt_cb(skb)->pkt_type);
> > +
> > + return 0;
>=20
> goto might be better to handle error here.
>=20
>=20
> --
> Gustavo F. Padovan
> ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.