* Re: [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module
From: Mark Brown @ 2014-09-09 11:27 UTC (permalink / raw)
To: Shengjiu Wang
Cc: alsa-devel, lgirdwood, tiwai, linux-kernel, timur, perex,
nicoleotsuka, Li.Xiubo, linuxppc-dev
In-Reply-To: <e5a50d8bfde3aea5fcd092118da5aea4072c7e2f.1410254125.git.shengjiu.wang@freescale.com>
[-- Attachment #1: Type: text/plain, Size: 694 bytes --]
On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:
> + if (ssi_private->soc->imx)
> + clk_prepare_enable(ssi_private->clk);
> +
We're ignoring the error code here.
> - ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> + ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
> if (IS_ERR(ssi_private->clk)) {
> ret = PTR_ERR(ssi_private->clk);
> - dev_err(&pdev->dev, "could not get clock: %d\n", ret);
> - return ret;
Why is this change being made? It wasn't mentioned in the commit log
and doesn't seem relevant to moving where the enable and disable are
done which is what the patch is supposed to be doing...
Please don't make different changes in the same patch.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply
* [PATCH] powerpc/iommu/ddw: Fix endianness
From: Alexey Kardashevskiy @ 2014-09-09 11:22 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Alexander Graf, Nishanth Aravamudan, Gavin Shan, linux-kernel,
Paul Mackerras, Anton Blanchard, Alistair Popple, linuxppc-dev
rtas_call() accepts and returns values in CPU endianness.
of_read_number() accepts big-endian values but create.addr_hi/lo returned
by rtas_call() are in CPU endiannes.
The dynamic_dma_window_prop struct defines all members as BE so let's
make it true.
struct dynamic_dma_window_prop {
__be32 liobn; /* tce table number */
__be64 dma_base; /* address hi,lo */
__be32 tce_shift; /* ilog2(tce_page_size) */
__be32 window_shift; /* ilog2(tce_window_size) */
};
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
arch/powerpc/platforms/pseries/iommu.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
index 7c1d77c..700020a 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -750,7 +750,7 @@ static void remove_ddw(struct device_node *np, bool remove_prop)
pr_debug("%s successfully cleared tces in window.\n",
np->full_name);
- ret = rtas_call(ddw_avail[2], 1, 1, NULL, liobn);
+ ret = rtas_call(be32_to_cpu(ddw_avail[2]), 1, 1, NULL, liobn);
if (ret)
pr_warning("%s: failed to remove direct window: rtas returned "
"%d to ibm,remove-pe-dma-window(%x) %llx\n",
@@ -842,7 +842,7 @@ static int query_ddw(struct pci_dev *dev, const u32 *ddw_avail,
cfg_addr = edev->pe_config_addr;
buid = edev->phb->buid;
- ret = rtas_call(ddw_avail[0], 3, 5, (u32 *)query,
+ ret = rtas_call(be32_to_cpu(ddw_avail[0]), 3, 5, (u32 *)query,
cfg_addr, BUID_HI(buid), BUID_LO(buid));
dev_info(&dev->dev, "ibm,query-pe-dma-windows(%x) %x %x %x"
" returned %d\n", ddw_avail[0], cfg_addr, BUID_HI(buid),
@@ -874,8 +874,9 @@ static int create_ddw(struct pci_dev *dev, const u32 *ddw_avail,
do {
/* extra outputs are LIOBN and dma-addr (hi, lo) */
- ret = rtas_call(ddw_avail[1], 5, 4, (u32 *)create, cfg_addr,
- BUID_HI(buid), BUID_LO(buid), page_shift, window_shift);
+ ret = rtas_call(be32_to_cpu(ddw_avail[1]), 5, 4, (u32 *)create,
+ cfg_addr, BUID_HI(buid), BUID_LO(buid),
+ page_shift, window_shift);
} while (rtas_busy_delay(ret));
dev_info(&dev->dev,
"ibm,create-pe-dma-window(%x) %x %x %x %x %x returned %d "
@@ -972,11 +973,11 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
dev_dbg(&dev->dev, "no free dynamic windows");
goto out_failed;
}
- if (be32_to_cpu(query.page_size) & 4) {
+ if (query.page_size & 4) {
page_shift = 24; /* 16MB */
- } else if (be32_to_cpu(query.page_size) & 2) {
+ } else if (query.page_size & 2) {
page_shift = 16; /* 64kB */
- } else if (be32_to_cpu(query.page_size) & 1) {
+ } else if (query.page_size & 1) {
page_shift = 12; /* 4kB */
} else {
dev_dbg(&dev->dev, "no supported direct page size in mask %x",
@@ -987,7 +988,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
/* verify the window * number of ptes will map the partition */
/* check largest block * page size > max memory hotplug addr */
max_addr = memory_hotplug_max();
- if (be32_to_cpu(query.largest_available_block) < (max_addr >> page_shift)) {
+ if (query.largest_available_block < (max_addr >> page_shift)) {
dev_dbg(&dev->dev, "can't map partiton max 0x%llx with %u "
"%llu-sized pages\n", max_addr, query.largest_available_block,
1ULL << page_shift);
@@ -1014,8 +1015,9 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
if (ret != 0)
goto out_free_prop;
- ddwprop->liobn = create.liobn;
- ddwprop->dma_base = cpu_to_be64(of_read_number(&create.addr_hi, 2));
+ ddwprop->liobn = cpu_to_be32(create.liobn);
+ ddwprop->dma_base = cpu_to_be64(((u64)create.addr_hi << 32) |
+ create.addr_lo);
ddwprop->tce_shift = cpu_to_be32(page_shift);
ddwprop->window_shift = cpu_to_be32(len);
@@ -1048,7 +1050,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
list_add(&window->list, &direct_window_list);
spin_unlock(&direct_window_list_lock);
- dma_addr = of_read_number(&create.addr_hi, 2);
+ dma_addr = be64_to_cpu(ddwprop->dma_base);
goto out_unlock;
out_free_window:
--
2.0.0
^ permalink raw reply related
* Re: bit fields && data tearing
From: Peter Hurley @ 2014-09-09 11:18 UTC (permalink / raw)
To: One Thousand Gnomes, H. Peter Anvin
Cc: Jakub Jelinek, linux-arch@vger.kernel.org, Tony Luck,
linux-ia64@vger.kernel.org, Oleg Nesterov,
linux-kernel@vger.kernel.org, David Laight, Paul Mackerras,
linux-alpha, Paul E. McKenney, linuxppc-dev@lists.ozlabs.org,
Miroslav Franc, Richard Henderson
In-Reply-To: <20140908201722.66bc6492@alan.etchedpixels.co.uk>
On 09/08/2014 03:17 PM, One Thousand Gnomes wrote:
>>> I think the whole "removing Alpha EV5" support is basically bonkers. Just
>>> use set_bit in the tty layer. Alpha will continue to work as well as it
>>> always has done and you won't design out support for any future processor
>>> that turns out not to do byte aligned stores.
>>>
>>> Alan
>>>
>>
>> Is *that* what we are talking about? I was added to this conversation
>> in the middle where it had already generalized, so I had no idea.
>>
>> -hpa
>
> Yes there are some flags in the tty layer that are vulnerable to this
> (although they've been vulnerable to it and missing a lock since last
> century with no known ill effects).
That observation cuts both ways; I'm willing to leave it vulnerable with
'no known ill effects' on the Alpha.
^ permalink raw reply
* Re: bit fields && data tearing
From: Peter Hurley @ 2014-09-09 11:14 UTC (permalink / raw)
To: H. Peter Anvin, One Thousand Gnomes
Cc: Jakub Jelinek, linux-arch@vger.kernel.org, Tony Luck,
linux-ia64@vger.kernel.org, Oleg Nesterov,
linux-kernel@vger.kernel.org, David Laight, Paul Mackerras,
linux-alpha, Paul E. McKenney, linuxppc-dev@lists.ozlabs.org,
Miroslav Franc, Richard Henderson
In-Reply-To: <540E3207.7090007@hurleysoftware.com>
On 09/08/2014 06:47 PM, Peter Hurley wrote:
> On 09/08/2014 01:59 PM, H. Peter Anvin wrote:
>> On 09/08/2014 10:52 AM, One Thousand Gnomes wrote:
>>> On Fri, 05 Sep 2014 08:41:52 -0700
>>> "H. Peter Anvin" <hpa@zytor.com> wrote:
>>>
>>>> On 09/05/2014 08:31 AM, Peter Hurley wrote:
>>>>>
>>>>> Which is a bit ironic because I remember when Digital had a team
>>>>> working on emulating native x86 apps on Alpha/NT.
>>>>>
>>>>
>>>> Right, because the x86 architecture was obsolete and would never scale...
>>>
>>> Talking about "not scaling" can anyone explain how a "you need to use
>>> set_bit() and friends" bug report scaled into a hundred message plus
>>> discussion about ambiguous properties of processors (and nobody has
>>> audited all the embedded platforms we support yet, or the weirder ARMs)
>>> and a propsal to remove Alpha support.
>>>
>>> Wouldn't it be *much* simpler to do what I suggested in the first place
>>> and use the existing intended for purpose, deliberately put there,
>>> functions for atomic bitops, because they are fast on sane processors and
>>> they work on everything else.
And much simpler how?
By turning a 4- line patch into a 400- line patch?
And what about multi-value assignments for which set_bit() doesn't work, like
the byte-sized ->ctrl_status field? Is the expectation to submit a patch
which fixes that for a system from 1995, but already works for everything else?
The extra complexity comes with real cost; reduced reliability for every other
arch .
>>> I think the whole "removing Alpha EV5" support is basically bonkers. Just
>>> use set_bit in the tty layer. Alpha will continue to work as well as it
>>> always has done and you won't design out support for any future processor
>>> that turns out not to do byte aligned stores.
I thought the overriding design principle of this kernel was to support
what exists now, not design-in support for the future which may have
different requirements than expected anyway.
>> Is *that* what we are talking about? I was added to this conversation
>> in the middle where it had already generalized, so I had no idea.
>
> No, this is just what brought this craziness to my attention.
>
> For example, byte- and short-sized circular buffers could not possibly
> be safe either, when the head nears the tail.
>
> Who has audited global storage and ensured that _every_ byte-sized write
> doesn't happen to be adjacent to some other storage that may not happen
> to be protected by the same (or any) lock?
And add to this list all bitfields because gcc ignores the type
specifier and only allocates the minimum number of bytes to contain the
declared fields.
Regards,
Peter Hurley
^ permalink raw reply
* RE: [PATCH v2] QE: move qe code from arch/powerpc to drivers/soc
From: qiang.zhao @ 2014-09-09 9:27 UTC (permalink / raw)
To: Scott Wood
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
Xiaobo Xie
In-Reply-To: <1409870809.24184.163.camel@snotra.buserror.net>
T24gRnJpLCAyMDE0LTA5LTA1IGF0IDA2OjQ3IEFNLCBXb29kIFNjb3R0IHdyb3RlOg0KDQoNCg0K
PiBTdWJqZWN0OiBSZTogW1BBVENIIHYyXSBRRTogbW92ZSBxZSBjb2RlIGZyb20gYXJjaC9wb3dl
cnBjIHRvIGRyaXZlcnMvc29jDQo+IA0KPiBPbiBUaHUsIDIwMTQtMDktMDQgYXQgMTM6MDYgKzA4
MDAsIFpoYW8gUWlhbmcgd3JvdGU6DQo+ID4gTFMxIGlzIGFybSBjcHUgYW5kIGl0IGhhcyBxZSBp
cCBibG9jay4NCj4gPiBtb3ZlIHFlIGNvZGUgZnJvbSBwbGF0Zm9ybSBkaXJlY3RvcnkgdG8gcHVi
bGljIGRpcmVjdG9yeS4NCj4gPg0KPiA+IFFFIGlzIGFuIElQIGJsb2NrIGludGVncmF0ZXMgc2V2
ZXJhbCBjb211bmljYXRpb25zIHBlcmlwaGVyYWwNCj4gPiBjb250cm9sbGVycy4gSXQgY2FuIGlt
cGxlbWVudCBhIHZhcmlldHkgb2YgYXBwbGljYXRpb25zLCBzdWNoIGFzIHVhcnQsDQo+ID4gdXNi
IGFuZCB0ZG0gYW5kIHNvIG9uLg0KPiA+DQo+ID4gU2lnbmVkLW9mZi1ieTogWmhhbyBRaWFuZyA8
QjQ1NDc1QGZyZWVzY2FsZS5jb20+DQo+ID4gLS0tDQo+ID4gQ2hhbmdlcyBmb3IgdjI6DQo+ID4g
CS0gbXYgY29kZSB0byBkcml2ZXJzL3NvYw0KPiANCj4gV2hvIHdpbGwgYmUgdGhlIG1haW50YWlu
ZXIgb2YgdGhpcyBjb2RlIG9uY2UgaXQgbGl2ZXMgaW4gZHJpdmVycy9zb2MsDQo+IGVzcGVjaWFs
bHkgb25jZSBpdCBpcyBubyBsb25nZXIgdXNlZCBvbmx5IGJ5IFBQQz8NCg0KSSBoYXZlIG5vIGlk
ZWEgYWJvdXQgdGhhdCwgY2FuIHlvdSBleHBsYWluIHdoeSB5b3Ugd2FudCB0byBrbm93IHdobyB3
aWxsIGJlIHRoZSBtYWludGFpbmVyLg0KDQo+IA0KPiA+ICA0NCBmaWxlcyBjaGFuZ2VkLCAxMTMg
aW5zZXJ0aW9ucygrKSwgMTEzIGRlbGV0aW9ucygtKSAgZGVsZXRlIG1vZGUNCj4gPiAxMDA2NDQg
YXJjaC9wb3dlcnBjL3N5c2Rldi9xZV9saWIvS2NvbmZpZw0KPiA+ICBjcmVhdGUgbW9kZSAxMDA2
NDQgZHJpdmVycy9zb2MvcWUvS2NvbmZpZyAgcmVuYW1lDQo+ID4ge2FyY2gvcG93ZXJwYy9zeXNk
ZXYvcWVfbGliID0+IGRyaXZlcnMvc29jL3FlfS9NYWtlZmlsZSAoMTAwJSkgIHJlbmFtZQ0KPiA+
IHthcmNoL3Bvd2VycGMvc3lzZGV2L3FlX2xpYiA9PiBkcml2ZXJzL3NvYy9xZX0vZ3Bpby5jICg5
OSUpICByZW5hbWUNCj4gPiB7YXJjaC9wb3dlcnBjL3N5c2Rldi9xZV9saWIgPT4gZHJpdmVycy9z
b2MvcWV9L3FlLmMgKDk5JSkgIHJlbmFtZQ0KPiA+IHthcmNoL3Bvd2VycGMvc3lzZGV2L3FlX2xp
YiA9PiBkcml2ZXJzL3NvYy9xZX0vcWVfaWMuYyAoOTklKSAgcmVuYW1lDQo+ID4ge2FyY2gvcG93
ZXJwYy9zeXNkZXYvcWVfbGliID0+IGRyaXZlcnMvc29jL3FlfS9xZV9pYy5oICg5OCUpICByZW5h
bWUNCj4gPiB7YXJjaC9wb3dlcnBjL3N5c2Rldi9xZV9saWIgPT4gZHJpdmVycy9zb2MvcWV9L3Fl
X2lvLmMgKDk5JSkgIHJlbmFtZQ0KPiA+IHthcmNoL3Bvd2VycGMvc3lzZGV2L3FlX2xpYiA9PiBk
cml2ZXJzL3NvYy9xZX0vdWNjLmMgKDk4JSkgIHJlbmFtZQ0KPiA+IHthcmNoL3Bvd2VycGMvc3lz
ZGV2L3FlX2xpYiA9PiBkcml2ZXJzL3NvYy9xZX0vdWNjX2Zhc3QuYyAoOTglKQ0KPiA+IHJlbmFt
ZSB7YXJjaC9wb3dlcnBjL3N5c2Rldi9xZV9saWIgPT4gZHJpdmVycy9zb2MvcWV9L3VjY19zbG93
LmMgKDk4JSkNCj4gPiByZW5hbWUge2FyY2gvcG93ZXJwYy9zeXNkZXYvcWVfbGliID0+IGRyaXZl
cnMvc29jL3FlfS91c2IuYyAoOTYlKQ0KPiANCj4gZHJpdmVycy9zb2MvZnNsLXFlDQo+IA0KPiA+
IGRpZmYgLS1naXQgYS9kcml2ZXJzL3NvYy9NYWtlZmlsZSBiL2RyaXZlcnMvc29jL01ha2VmaWxl
IGluZGV4DQo+ID4gMGY3YzQ0Ny4uNWRhMWE0ODIgMTAwNjQ0DQo+ID4gLS0tIGEvZHJpdmVycy9z
b2MvTWFrZWZpbGUNCj4gPiArKysgYi9kcml2ZXJzL3NvYy9NYWtlZmlsZQ0KPiA+IEBAIC0zLDMg
KzMsNSBAQA0KPiA+ICAjDQo+ID4NCj4gPiAgb2JqLSQoQ09ORklHX0FSQ0hfUUNPTSkJCSs9IHFj
b20vDQo+ID4gKw0KPiA+ICtvYmotJChDT05GSUdfUVVJQ0NfRU5HSU5FKSAgICAgKz0gcWUvDQo+
IA0KPiBQbGVhc2Uga2VlcCB0aGUgZmlsZSBjb25zaXN0ZW50IHJlZ2FyZGluZyB0YWJzIHZlcnN1
cyBzcGFjZXMuDQo+IA0KPiBQbHVzLCB3aHkgZG8geW91IG5lZWQgYSBuZXdsaW5lIGJldHdlZW4g
dGhlbT8NCg0KSSB3aWxsIG1vZGlmeSBpdCBpbiB2Mw0KDQo+IA0KPiA+IGRpZmYgLS1naXQgYS9k
cml2ZXJzL3NvYy9xZS9LY29uZmlnIGIvZHJpdmVycy9zb2MvcWUvS2NvbmZpZyBuZXcgZmlsZQ0K
PiA+IG1vZGUgMTAwNjQ0IGluZGV4IDAwMDAwMDAuLjhiMDNjYTINCj4gPiAtLS0gL2Rldi9udWxs
DQo+ID4gKysrIGIvZHJpdmVycy9zb2MvcWUvS2NvbmZpZw0KPiA+IEBAIC0wLDAgKzEsNDUgQEAN
Cj4gPiArIw0KPiA+ICsjIFFFIENvbW11bmljYXRpb24gb3B0aW9ucw0KPiA+ICsjDQo+ID4gK2Nv
bmZpZyBRVUlDQ19FTkdJTkUNCj4gPiArCWJvb2wgIkZyZWVzY2FsZSBRVUlDQyBFbmdpbmUgKFFF
KSBTdXBwb3J0Ig0KPiA+ICsJZGVwZW5kcyBvbiBGU0xfU09DICYmIFBQQzMyDQo+ID4gKwlzZWxl
Y3QgUFBDX0xJQl9SSEVBUA0KPiA+ICsJc2VsZWN0IENSQzMyDQo+ID4gKwloZWxwDQo+ID4gKwkg
IFRoZSBRVUlDQyBFbmdpbmUgKFFFKSBpcyBhIG5ldyBnZW5lcmF0aW9uIG9mIGNvbW11bmljYXRp
b25zDQo+ID4gKwkgIGNvcHJvY2Vzc29ycyBvbiBGcmVlc2NhbGUgZW1iZWRkZWQgQ1BVcyAoYWtp
biB0byBDUE0gaW4gb2xkZXINCj4gY2hpcHMpLg0KPiA+ICsJICBTZWxlY3RpbmcgdGhpcyBvcHRp
b24gbWVhbnMgdGhhdCB5b3Ugd2lzaCB0byBidWlsZCBhIGtlcm5lbA0KPiA+ICsJICBmb3IgYSBt
YWNoaW5lIHdpdGggYSBRRSBjb3Byb2Nlc3Nvci4NCj4gPiArDQo+ID4gK2NvbmZpZyBRRV9HUElP
DQo+ID4gKwlib29sICJRRSBHUElPIHN1cHBvcnQiDQo+ID4gKwlkZXBlbmRzIG9uIFFVSUNDX0VO
R0lORQ0KPiA+ICsJc2VsZWN0IEFSQ0hfUkVRVUlSRV9HUElPTElCDQo+ID4gKwloZWxwDQo+ID4g
KwkgIFNheSBZIGhlcmUgaWYgeW91J3JlIGdvaW5nIHRvIHVzZSBoYXJkd2FyZSB0aGF0IGNvbm5l
Y3RzIHRvIHRoZQ0KPiA+ICsJICBRRSBHUElPcy4NCj4gPiArDQo+ID4gK2NvbmZpZyBVQ0NfU0xP
Vw0KPiA+ICsJYm9vbA0KPiA+ICsJZGVmYXVsdCB5IGlmIFNFUklBTF9RRQ0KPiA+ICsJaGVscA0K
PiA+ICsJICBUaGlzIG9wdGlvbiBwcm92aWRlcyBxZV9saWIgc3VwcG9ydCB0byBVQ0Mgc2xvdw0K
PiA+ICsJICBwcm90b2NvbHM6IFVBUlQsIEJJU1lOQywgUU1DDQo+ID4gKw0KPiA+ICtjb25maWcg
VUNDX0ZBU1QNCj4gPiArCWJvb2wNCj4gPiArCWRlZmF1bHQgeSBpZiBVQ0NfR0VUSA0KPiA+ICsJ
aGVscA0KPiA+ICsJICBUaGlzIG9wdGlvbiBwcm92aWRlcyBxZV9saWIgc3VwcG9ydCB0byBVQ0Mg
ZmFzdA0KPiA+ICsJICBwcm90b2NvbHM6IEhETEMsIEV0aGVybmV0LCBBVE0sIHRyYW5zcGFyZW50
DQo+ID4gKw0KPiA+ICtjb25maWcgVUNDDQo+ID4gKwlib29sDQo+ID4gKwlkZWZhdWx0IHkgaWYg
VUNDX0ZBU1QgfHwgVUNDX1NMT1cNCj4gPiArDQo+ID4gK2NvbmZpZyBRRV9VU0INCj4gPiArCWJv
b2wNCj4gPiArCWRlZmF1bHQgeSBpZiBVU0JfRlNMX1FFDQo+ID4gKwloZWxwDQo+ID4gKwkgIFFF
IFVTQiBDb250cm9sbGVyIHN1cHBvcnQNCj4gDQo+IEZpcnN0IGNvdWxkIHdlIGdpdmUgdGhlc2Ug
bmFtZXMgYmV0dGVyIG5hbWVzcGFjaW5nPw0KDQpBZGQgRlNMIGFzIHByZWZpeD8NCg0KPiANCj4g
LVNjb3R0DQo+IA0KUmVnYXJkcywNClpoYW8gUWlhbmcNCg==
^ permalink raw reply
* Re: bit fields && data tearing
From: Peter Hurley @ 2014-09-09 10:40 UTC (permalink / raw)
To: James Bottomley
Cc: Jakub Jelinek, One Thousand Gnomes, linux-arch, linux-ia64,
Mikael Pettersson, Oleg Nesterov, linux-kernel, Tony Luck,
Paul Mackerras, H. Peter Anvin, paulmck, linuxppc-dev,
Miroslav Franc, Richard Henderson
In-Reply-To: <1410231392.2028.15.camel@jarvis.lan>
On 09/08/2014 10:56 PM, James Bottomley wrote:
> On Mon, 2014-09-08 at 19:30 -0400, Peter Hurley wrote:
>> On 09/08/2014 01:50 AM, James Bottomley wrote:
>>>> But additionally, even if gcc combines adjacent writes _that are part
>>>> of the program flow_ then I believe the situation is no worse than
>>>> would otherwise exist.
>>>>
>>>> For instance, given the following:
>>>>
>>>> struct x {
>>>> spinlock_t lock;
>>>> long a;
>>>> byte b;
>>>> byte c;
>>>> };
>>>>
>>>> void locked_store_b(struct x *p)
>>>> {
>>>> spin_lock(&p->lock);
>>>> p->b = 1;
>>>> spin_unlock(&p->lock);
>>>> p->c = 2;
>>>> }
>>>>
>>>> Granted, the author probably expects ordered writes of
>>>> STORE B
>>>> STORE C
>>>> but that's not guaranteed because there is no memory barrier
>>>> ordering B before C.
>>>
>>> Yes, there is: loads and stores may not migrate into or out of critical
>>> sections.
>>
>> That's a common misconception.
>>
>> The processor is free to re-order this to:
>>
>> STORE C
>> STORE B
>> UNLOCK
>>
>> That's because the unlock() only guarantees that:
>>
>> Stores before the unlock in program order are guaranteed to complete
>> before the unlock completes. Stores after the unlock _may_ complete
>> before the unlock completes.
>>
>> My point was that even if compiler barriers had the same semantics
>> as memory barriers, the situation would be no worse. That is, code
>> that is sensitive to memory barriers (like the example I gave above)
>> would merely have the same fragility with one-way compiler barriers
>> (with respect to the compiler combining writes).
>>
>> That's what I meant by "no worse than would otherwise exist".
>
> Actually, that's not correct. This is actually deja vu with me on the
> other side of the argument. When we first did spinlocks on PA, I argued
> as you did: lock only a barrier for code after and unlock for code
> before. The failing case is that you can have a critical section which
> performs an atomically required operation and a following unit which
> depends on it being performed. If you begin the following unit before
> the atomic requirement, you may end up losing. It turns out this kind
> of pattern is inherent in a lot of mail box device drivers: you need to
> set up the mailbox atomically then poke it. Setup is usually atomic,
> deciding which mailbox to prime and actually poking it is in the
> following unit. Priming often involves an I/O bus transaction and if
> you poke before priming, you get a misfire.
Take it up with the man because this was discussed extensively last
year and it was decided that unlocks would not be full barriers.
Thus the changes to memory-barriers.txt that explicitly note this
and the addition of smp_mb__after_unlock_lock() (for two different
locks; an unlock followed by a lock on the same lock is a full barrier).
Code that expects ordered writes after an unlock needs to explicitly
add the memory barrier.
Regards,
Peter Hurley
^ permalink raw reply
* RE: [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module
From: Li.Xiubo @ 2014-09-09 9:55 UTC (permalink / raw)
To: shengjiu.wang@freescale.com, timur@tabi.org,
nicoleotsuka@gmail.com, lgirdwood@gmail.com, broonie@kernel.org,
perex@perex.cz, tiwai@suse.de
Cc: alsa-devel@alsa-project.org, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
In-Reply-To: <e5a50d8bfde3aea5fcd092118da5aea4072c7e2f.1410254125.git.shengjiu.wang@freescale.com>
Hi,
> Subject: [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module
>=20
> Move the ipg clock enable and disable operation to startup and shutdown,
> that is only enable ipg clock when ssi is working. we don't need to enabl=
e
> ipg clock in probe.
> Another register accessing need the ipg clock, so use
> devm_regmap_init_mmio_clk
> instead of devm_regmap_init_mmio.
>
You should split this into two separate patches, which will be more
Easy to be reviewed.
=20
Thanks,
BRs
Xiubo
^ permalink raw reply
* Re: [alsa-devel] [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module
From: Markus Pargmann @ 2014-09-09 9:49 UTC (permalink / raw)
To: Shengjiu Wang
Cc: alsa-devel, lgirdwood, tiwai, Li.Xiubo, timur, perex,
nicoleotsuka, broonie, linuxppc-dev, linux-kernel
In-Reply-To: <e5a50d8bfde3aea5fcd092118da5aea4072c7e2f.1410254125.git.shengjiu.wang@freescale.com>
[-- Attachment #1: Type: text/plain, Size: 2953 bytes --]
Hi,
On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:
> Move the ipg clock enable and disable operation to startup and shutdown,
> that is only enable ipg clock when ssi is working. we don't need to enable
> ipg clock in probe.
> Another register accessing need the ipg clock, so use devm_regmap_init_mmio_clk
> instead of devm_regmap_init_mmio.
>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> ---
> sound/soc/fsl/fsl_ssi.c | 38 +++++++++++++++++++++++++++-----------
> 1 file changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 2fc3e66..d32d0f5 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -531,6 +531,9 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream,
> struct fsl_ssi_private *ssi_private =
> snd_soc_dai_get_drvdata(rtd->cpu_dai);
>
> + if (ssi_private->soc->imx)
> + clk_prepare_enable(ssi_private->clk);
> +
> /* When using dual fifo mode, it is safer to ensure an even period
> * size. If appearing to an odd number while DMA always starts its
> * task from fifo0, fifo1 would be neglected at the end of each
> @@ -544,6 +547,22 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream,
> }
>
> /**
> + * fsl_ssi_shutdown: shutdown the SSI
> + *
> + */
> +static void fsl_ssi_shutdown(struct snd_pcm_substream *substream,
> + struct snd_soc_dai *dai)
> +{
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + struct fsl_ssi_private *ssi_private =
> + snd_soc_dai_get_drvdata(rtd->cpu_dai);
> +
> + if (ssi_private->soc->imx)
> + clk_disable_unprepare(ssi_private->clk);
> +
> +}
> +
> +/**
> * fsl_ssi_set_bclk - configure Digital Audio Interface bit clock
> *
> * Note: This function can be only called when using SSI as DAI master
> @@ -1043,6 +1062,7 @@ static int fsl_ssi_dai_probe(struct snd_soc_dai *dai)
>
> static const struct snd_soc_dai_ops fsl_ssi_dai_ops = {
> .startup = fsl_ssi_startup,
> + .shutdown = fsl_ssi_shutdown,
> .hw_params = fsl_ssi_hw_params,
> .hw_free = fsl_ssi_hw_free,
> .set_fmt = fsl_ssi_set_dai_fmt,
> @@ -1168,16 +1188,10 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
> u32 dmas[4];
> int ret;
>
> - ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
> + ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
This does not work for most imx SoCs at the moment. imx27, imx35, imx51
etc. do not have clock-names defined in the devicetree.
Best regards,
Markus
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH v2 2/3] sched: Add helper for task stack page overrun checking
From: Aaron Tomlin @ 2014-09-09 9:42 UTC (permalink / raw)
To: peterz
Cc: dzickus, jcastillo, riel, x86, akpm, minchan, bmr, prarit, oleg,
rostedt, linux-kernel, hannes, mingo, aneesh.kumar, akpm, atomlin,
jgh, linuxppc-dev, tglx, pzijlstr
In-Reply-To: <1410255749-2956-1-git-send-email-atomlin@redhat.com>
This facility is used in a few places so let's introduce
a helper function to improve code readability.
Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
arch/powerpc/mm/fault.c | 4 +---
arch/x86/mm/fault.c | 4 +---
include/linux/sched.h | 2 ++
kernel/trace/trace_stack.c | 2 +-
4 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 35d0760c..99b2f27 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -507,7 +507,6 @@ bail:
void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
{
const struct exception_table_entry *entry;
- unsigned long *stackend;
/* Are we prepared to handle this fault? */
if ((entry = search_exception_tables(regs->nip)) != NULL) {
@@ -536,8 +535,7 @@ void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
printk(KERN_ALERT "Faulting instruction address: 0x%08lx\n",
regs->nip);
- stackend = end_of_stack(current);
- if (*stackend != STACK_END_MAGIC)
+ if (task_stack_end_corrupted(current))
printk(KERN_ALERT "Thread overran stack, or stack corrupted\n");
die("Kernel access of bad area", regs, sig);
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index bc23a70..6240bc7 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -648,7 +648,6 @@ no_context(struct pt_regs *regs, unsigned long error_code,
unsigned long address, int signal, int si_code)
{
struct task_struct *tsk = current;
- unsigned long *stackend;
unsigned long flags;
int sig;
@@ -708,8 +707,7 @@ no_context(struct pt_regs *regs, unsigned long error_code,
show_fault_oops(regs, error_code, address);
- stackend = end_of_stack(tsk);
- if (*stackend != STACK_END_MAGIC)
+ if (task_stack_end_corrupted(tsk))
printk(KERN_EMERG "Thread overran stack, or stack corrupted\n");
tsk->thread.cr2 = address;
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 4dee9d7..6e07cb9 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2615,6 +2615,8 @@ static inline unsigned long *end_of_stack(struct task_struct *p)
}
#endif
+#define task_stack_end_corrupted(task) \
+ (*(end_of_stack(task)) != STACK_END_MAGIC)
static inline int object_is_on_stack(void *obj)
{
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 1636e41..16eddb3 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -170,7 +170,7 @@ check_stack(unsigned long ip, unsigned long *stack)
i++;
}
- if (*end_of_stack(current) != STACK_END_MAGIC) {
+ if (task_stack_end_corrupted(current)) {
print_max_stack();
BUG();
}
--
1.9.3
^ permalink raw reply related
* [PATCH v2 1/3] init/main.c: Give init_task a canary
From: Aaron Tomlin @ 2014-09-09 9:42 UTC (permalink / raw)
To: peterz
Cc: dzickus, jcastillo, riel, x86, akpm, minchan, bmr, prarit, oleg,
rostedt, linux-kernel, hannes, mingo, aneesh.kumar, akpm, atomlin,
jgh, linuxppc-dev, tglx, pzijlstr
In-Reply-To: <1410255749-2956-1-git-send-email-atomlin@redhat.com>
Tasks get their end of stack set to STACK_END_MAGIC with the
aim to catch stack overruns. Currently this feature does not
apply to init_task. This patch removes this restriction.
Note that a similar patch was posted by Prarit Bhargava [1]
some time ago but was never merged.
[1]: http://marc.info/?l=linux-kernel&m=127144305403241&w=2
Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
arch/powerpc/mm/fault.c | 3 +--
arch/x86/mm/fault.c | 3 +--
include/linux/sched.h | 2 ++
init/main.c | 1 +
kernel/fork.c | 12 +++++++++---
kernel/trace/trace_stack.c | 4 +---
6 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 51ab9e7..35d0760c 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -30,7 +30,6 @@
#include <linux/kprobes.h>
#include <linux/kdebug.h>
#include <linux/perf_event.h>
-#include <linux/magic.h>
#include <linux/ratelimit.h>
#include <linux/context_tracking.h>
@@ -538,7 +537,7 @@ void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
regs->nip);
stackend = end_of_stack(current);
- if (current != &init_task && *stackend != STACK_END_MAGIC)
+ if (*stackend != STACK_END_MAGIC)
printk(KERN_ALERT "Thread overran stack, or stack corrupted\n");
die("Kernel access of bad area", regs, sig);
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index a241946..bc23a70 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -3,7 +3,6 @@
* Copyright (C) 2001, 2002 Andi Kleen, SuSE Labs.
* Copyright (C) 2008-2009, Red Hat Inc., Ingo Molnar
*/
-#include <linux/magic.h> /* STACK_END_MAGIC */
#include <linux/sched.h> /* test_thread_flag(), ... */
#include <linux/kdebug.h> /* oops_begin/end, ... */
#include <linux/module.h> /* search_exception_table */
@@ -710,7 +709,7 @@ no_context(struct pt_regs *regs, unsigned long error_code,
show_fault_oops(regs, error_code, address);
stackend = end_of_stack(tsk);
- if (tsk != &init_task && *stackend != STACK_END_MAGIC)
+ if (*stackend != STACK_END_MAGIC)
printk(KERN_EMERG "Thread overran stack, or stack corrupted\n");
tsk->thread.cr2 = address;
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 5c2c885..4dee9d7 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -57,6 +57,7 @@ struct sched_param {
#include <linux/llist.h>
#include <linux/uidgid.h>
#include <linux/gfp.h>
+#include <linux/magic.h>
#include <asm/processor.h>
@@ -2636,6 +2637,7 @@ static inline unsigned long stack_not_used(struct task_struct *p)
return (unsigned long)n - (unsigned long)end_of_stack(p);
}
#endif
+extern void task_stack_end_magic(struct task_struct *tsk);
/* set thread flags in other task's structures
* - see asm/thread_info.h for TIF_xxxx flags available
diff --git a/init/main.c b/init/main.c
index bb1aed9..bcdabf3a 100644
--- a/init/main.c
+++ b/init/main.c
@@ -508,6 +508,7 @@ asmlinkage __visible void __init start_kernel(void)
* lockdep hash:
*/
lockdep_init();
+ task_stack_end_magic(&init_task);
smp_setup_processor_id();
debug_objects_early_init();
diff --git a/kernel/fork.c b/kernel/fork.c
index 0cf9cdb..67cb1e3 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -294,11 +294,18 @@ int __weak arch_dup_task_struct(struct task_struct *dst,
return 0;
}
+void task_stack_end_magic(struct task_struct *tsk)
+{
+ unsigned long *stackend;
+
+ stackend = end_of_stack(tsk);
+ *stackend = STACK_END_MAGIC; /* for overflow detection */
+}
+
static struct task_struct *dup_task_struct(struct task_struct *orig)
{
struct task_struct *tsk;
struct thread_info *ti;
- unsigned long *stackend;
int node = tsk_fork_get_node(orig);
int err;
@@ -328,8 +335,7 @@ static struct task_struct *dup_task_struct(struct task_struct *orig)
setup_thread_stack(tsk, orig);
clear_user_return_notifier(tsk);
clear_tsk_need_resched(tsk);
- stackend = end_of_stack(tsk);
- *stackend = STACK_END_MAGIC; /* for overflow detection */
+ task_stack_end_magic(tsk);
#ifdef CONFIG_CC_STACKPROTECTOR
tsk->stack_canary = get_random_int();
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 8a4e5cb..1636e41 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -13,7 +13,6 @@
#include <linux/sysctl.h>
#include <linux/init.h>
#include <linux/fs.h>
-#include <linux/magic.h>
#include <asm/setup.h>
@@ -171,8 +170,7 @@ check_stack(unsigned long ip, unsigned long *stack)
i++;
}
- if ((current != &init_task &&
- *(end_of_stack(current)) != STACK_END_MAGIC)) {
+ if (*end_of_stack(current) != STACK_END_MAGIC) {
print_max_stack();
BUG();
}
--
1.9.3
^ permalink raw reply related
* [PATCH v2 3/3] sched: BUG when stack end location is over written
From: Aaron Tomlin @ 2014-09-09 9:42 UTC (permalink / raw)
To: peterz
Cc: dzickus, jcastillo, riel, x86, akpm, minchan, bmr, prarit, oleg,
rostedt, linux-kernel, hannes, mingo, aneesh.kumar, akpm, atomlin,
jgh, linuxppc-dev, tglx, pzijlstr
In-Reply-To: <1410255749-2956-1-git-send-email-atomlin@redhat.com>
Currently in the event of a stack overrun a call to schedule()
does not check for this type of corruption. This corruption is
often silent and can go unnoticed. However once the corrupted
region is examined at a later stage, the outcome is undefined
and often results in a sporadic page fault which cannot be
handled.
This patch checks for a stack overrun and takes appropriate
action since the damage is already done, there is no point
in continuing.
Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
kernel/sched/core.c | 4 ++++
lib/Kconfig.debug | 12 ++++++++++++
2 files changed, 16 insertions(+)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index ec1a286..182b2d1 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2660,6 +2660,10 @@ static noinline void __schedule_bug(struct task_struct *prev)
*/
static inline void schedule_debug(struct task_struct *prev)
{
+#ifdef CONFIG_SCHED_STACK_END_CHECK
+ if (unlikely(task_stack_end_corrupted(prev)))
+ BUG();
+#endif
/*
* Test if we are atomic. Since do_exit() needs to call into
* schedule() atomically, we ignore that path. Otherwise whine
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index a285900..2a8280a 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -824,6 +824,18 @@ config SCHEDSTATS
application, you can say N to avoid the very slight overhead
this adds.
+config SCHED_STACK_END_CHECK
+ bool "Detect stack corruption on calls to schedule()"
+ depends on DEBUG_KERNEL
+ default y
+ help
+ This option checks for a stack overrun on calls to schedule().
+ If the stack end location is found to be over written always panic as
+ the content of the corrupted region can no longer be trusted.
+ This is to ensure no erroneous behaviour occurs which could result in
+ data corruption or a sporadic crash at a later stage once the region
+ is examined. The runtime overhead introduced is minimal.
+
config TIMER_STATS
bool "Collect kernel timers statistics"
depends on DEBUG_KERNEL && PROC_FS
--
1.9.3
^ permalink raw reply related
* [PATCH v2 0/3] sched: Always check the integrity of the canary
From: Aaron Tomlin @ 2014-09-09 9:42 UTC (permalink / raw)
To: peterz
Cc: dzickus, jcastillo, riel, x86, akpm, minchan, bmr, prarit, oleg,
rostedt, linux-kernel, hannes, mingo, aneesh.kumar, akpm, atomlin,
jgh, linuxppc-dev, tglx, pzijlstr
In-Reply-To: <20140904161124.GD1436@atomlin.usersys.redhat.com>
Resending with "v2" added to each subject line:
Currently in the event of a stack overrun a call to schedule()
does not check for this type of corruption. This corruption is
often silent and can go unnoticed. However once the corrupted
region is examined at a later stage, the outcome is undefined
and often results in a sporadic page fault which cannot be
handled.
The first patch adds a canary to init_task's end of stack.
While the second patch provides a helper to determine the
integrity of the canary. The third checks for a stack
overrun and takes appropriate action since the damage
is already done, there is no point in continuing.
Changes since v1:
* Rebased against v3.17-rc4
* Add a canary to init_task - Oleg Nesterov
* Fix various code formatting issues - Peter Zijlstra
* Introduce Kconfig option - Peter Zijlstra
Aaron Tomlin (3):
init/main.c: Give init_task a canary
sched: Add helper for task stack page overrun checking
sched: BUG when stack end location is over written
arch/powerpc/mm/fault.c | 5 +----
arch/x86/mm/fault.c | 5 +----
include/linux/sched.h | 4 ++++
init/main.c | 1 +
kernel/fork.c | 12 +++++++++---
kernel/sched/core.c | 4 ++++
kernel/trace/trace_stack.c | 4 +---
lib/Kconfig.debug | 12 ++++++++++++
8 files changed, 33 insertions(+), 14 deletions(-)
--
1.9.3
^ permalink raw reply
* [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module
From: Shengjiu Wang @ 2014-09-09 9:18 UTC (permalink / raw)
To: timur, nicoleotsuka, Li.Xiubo, lgirdwood, broonie, perex, tiwai
Cc: alsa-devel, linuxppc-dev, linux-kernel
Move the ipg clock enable and disable operation to startup and shutdown,
that is only enable ipg clock when ssi is working. we don't need to enable
ipg clock in probe.
Another register accessing need the ipg clock, so use devm_regmap_init_mmio_clk
instead of devm_regmap_init_mmio.
Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
---
sound/soc/fsl/fsl_ssi.c | 38 +++++++++++++++++++++++++++-----------
1 file changed, 27 insertions(+), 11 deletions(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 2fc3e66..d32d0f5 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -531,6 +531,9 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream,
struct fsl_ssi_private *ssi_private =
snd_soc_dai_get_drvdata(rtd->cpu_dai);
+ if (ssi_private->soc->imx)
+ clk_prepare_enable(ssi_private->clk);
+
/* When using dual fifo mode, it is safer to ensure an even period
* size. If appearing to an odd number while DMA always starts its
* task from fifo0, fifo1 would be neglected at the end of each
@@ -544,6 +547,22 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream,
}
/**
+ * fsl_ssi_shutdown: shutdown the SSI
+ *
+ */
+static void fsl_ssi_shutdown(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct snd_soc_pcm_runtime *rtd = substream->private_data;
+ struct fsl_ssi_private *ssi_private =
+ snd_soc_dai_get_drvdata(rtd->cpu_dai);
+
+ if (ssi_private->soc->imx)
+ clk_disable_unprepare(ssi_private->clk);
+
+}
+
+/**
* fsl_ssi_set_bclk - configure Digital Audio Interface bit clock
*
* Note: This function can be only called when using SSI as DAI master
@@ -1043,6 +1062,7 @@ static int fsl_ssi_dai_probe(struct snd_soc_dai *dai)
static const struct snd_soc_dai_ops fsl_ssi_dai_ops = {
.startup = fsl_ssi_startup,
+ .shutdown = fsl_ssi_shutdown,
.hw_params = fsl_ssi_hw_params,
.hw_free = fsl_ssi_hw_free,
.set_fmt = fsl_ssi_set_dai_fmt,
@@ -1168,16 +1188,10 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
u32 dmas[4];
int ret;
- ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
+ ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
if (IS_ERR(ssi_private->clk)) {
ret = PTR_ERR(ssi_private->clk);
- dev_err(&pdev->dev, "could not get clock: %d\n", ret);
- return ret;
- }
-
- ret = clk_prepare_enable(ssi_private->clk);
- if (ret) {
- dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret);
+ dev_err(&pdev->dev, "could not get ipg clock: %d\n", ret);
return ret;
}
@@ -1236,7 +1250,6 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
return 0;
error_pcm:
- clk_disable_unprepare(ssi_private->clk);
return ret;
}
@@ -1246,7 +1259,6 @@ static void fsl_ssi_imx_clean(struct platform_device *pdev,
{
if (!ssi_private->use_dma)
imx_pcm_fiq_exit(pdev);
- clk_disable_unprepare(ssi_private->clk);
}
static int fsl_ssi_probe(struct platform_device *pdev)
@@ -1321,7 +1333,11 @@ static int fsl_ssi_probe(struct platform_device *pdev)
return -ENOMEM;
}
- ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
+ if (ssi_private->soc->imx)
+ ssi_private->regs = devm_regmap_init_mmio_clk(&pdev->dev,
+ "ipg", iomem, &fsl_ssi_regconfig);
+ else
+ ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
&fsl_ssi_regconfig);
if (IS_ERR(ssi_private->regs)) {
dev_err(&pdev->dev, "Failed to init register map\n");
--
1.7.9.5
^ permalink raw reply related
* Re: bit fields && data tearing
From: Arnd Bergmann @ 2014-09-09 8:11 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Jakub Jelinek, One Thousand Gnomes, Tony Luck, linux-ia64,
Peter Hurley, Mikael Pettersson, Oleg Nesterov, Chris Metcalf,
linux-kernel, James Bottomley, Paul Mackerras, linux-arch,
paulmck, linuxppc-dev, Miroslav Franc, Richard Henderson
In-Reply-To: <540E6582.9020403@zytor.com>
On Monday 08 September 2014 19:27:14 H. Peter Anvin wrote:
> On 09/08/2014 03:43 PM, James Bottomley wrote:
> >
> > This was years ago (possibly decades). We had to implement in-kernel
> > unaligned traps for the networking layer because it could access short
> > and int fields that weren't of the correct alignment when processing
> > packets. It that's all corrected now, we wouldn't really notice (except
> > a bit of a speed up since an unaligned trap effectively places the
> > broken out instructions into the bit stream).
I believe the current line of thinking is to consider it a bug in
the device driver. Those bugs may still exist in some rare legacy
drivers, but anything you'd see in practice should not require unaligned
access any more.
> Well, ARM doesn't trap, it just silently gives garbage on unaligned
> memory references.
ARMv6/v7 (anything that really matters these days) has efficient unaligned
accesses that work. Older CPUs are normally set to trap on unaligned
access, originally for the reason James mentions above, but there are
some even older machines that rely on abusing unaligned accesses to do
byte swapping.
Arnd
^ permalink raw reply
* Re: [PATCH 2/2 v5] powerpc/kvm: common sw breakpoint instr across ppc
From: Madhavan Srinivasan @ 2014-09-09 7:41 UTC (permalink / raw)
To: Alexander Graf, benh, paulus, mpe; +Cc: linuxppc-dev, kvm-ppc, kvm
In-Reply-To: <540DAA9E.1070102@suse.de>
On Monday 08 September 2014 06:39 PM, Alexander Graf wrote:
>
>
> On 07.09.14 18:31, Madhavan Srinivasan wrote:
>> This patch extends the use of illegal instruction as software
>> breakpoint instruction across the ppc platform. Patch extends
>> booke program interrupt code to support software breakpoint.
>>
>> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>> ---
>>
>> Patch is only compile tested. Will really help if
>> someone can try it out and let me know comments.
>>
>> arch/powerpc/kvm/booke.c | 18 ++++++++++++++++--
>> 1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
>> index b4c89fa..1b84853 100644
>> --- a/arch/powerpc/kvm/booke.c
>> +++ b/arch/powerpc/kvm/booke.c
>> @@ -870,6 +870,10 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>> case BOOKE_INTERRUPT_HV_PRIV:
>> emulated = kvmppc_get_last_inst(vcpu, false, &last_inst);
>> break;
>> + case BOOKE_INTERRUPT_PROGRAM:
>> + /*SW breakpoints arrive as illegal instructions on HV */
>
> Is it my email client or is there a space missing again? ;)
>
Facepalm. Will fix it.
> Also, please only fetch the last instruction if debugging is active.
>
Will change it.
>> + emulated = kvmppc_get_last_inst(vcpu, false, &last_inst);
>> + break;
>> default:
>> break;
>> }
>> @@ -947,7 +951,17 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>> break;
>>
>> case BOOKE_INTERRUPT_PROGRAM:
>> - if (vcpu->arch.shared->msr & (MSR_PR | MSR_GS)) {
>> + if ((vcpu->arch.shared->msr & (MSR_PR | MSR_GS)) &&
>> + (last_inst == KVMPPC_INST_SW_BREAKPOINT)) {
>
> I think this is changing the logic from "if the guest is in user mode or
> we're in HV, deflect" to "if the guest is in user mode or an HV guest
> and the instruction is a breakpoint, treat it as debug. Otherwise
> deflect". So you're essentially breaking PR KVM here from what I can tell.
>
> Why don't you just split the whole thing out to the beginning of
> BOOKE_INTERRUPT_PROGRAM and check for
>
> a) debug is enabled
> b) instruction is sw breakpoint
>
This is what we pretty much do for the server side. Will changes it.
> instead?
>
>> + /*
>> + * We are here because of an SW breakpoint instr,
>> + * so lets return to host to handle.
>> + */
>> + r = kvmppc_handle_debug(run, vcpu);
>> + run->exit_reason = KVM_EXIT_DEBUG;
>> + kvmppc_account_exit(vcpu, DEBUG_EXITS);
>> + break;
>> + } else {
>> /*
>> * Program traps generated by user-level software must
>> * be handled by the guest kernel.
>> @@ -1505,7 +1519,7 @@ int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
>> val = get_reg_val(reg->id, vcpu->arch.tsr);
>> break;
>> case KVM_REG_PPC_DEBUG_INST:
>> - val = get_reg_val(reg->id, KVMPPC_INST_EHPRIV_DEBUG);
>
> Please also remove the definition of EHPRIV_DEBUG.
>
OK. Will do.
Thanks for review
Maddy
>
> Alex
>
>> + val = get_reg_val(reg->id, KVMPPC_INST_SW_BREAKPOINT);
>> break;
>> case KVM_REG_PPC_VRSAVE:
>> val = get_reg_val(reg->id, vcpu->arch.vrsave);
>>
>
^ permalink raw reply
* Re: [PATCH 1/2 v5] powerpc/kvm: support to handle sw breakpoint
From: Madhavan Srinivasan @ 2014-09-09 7:31 UTC (permalink / raw)
To: Alexander Graf, benh, paulus, mpe; +Cc: linuxppc-dev, kvm-ppc, kvm
In-Reply-To: <540DA980.1050708@suse.de>
On Monday 08 September 2014 06:35 PM, Alexander Graf wrote:
>
>
> On 07.09.14 18:31, Madhavan Srinivasan wrote:
>> This patch adds kernel side support for software breakpoint.
>> Design is that, by using an illegal instruction, we trap to hypervisor
>> via Emulation Assistance interrupt, where we check for the illegal instruction
>> and accordingly we return to Host or Guest. Patch also adds support for
>> software breakpoint in PR KVM.
>>
>> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/include/asm/kvm_ppc.h | 6 ++++++
>> arch/powerpc/kvm/book3s.c | 3 ++-
>> arch/powerpc/kvm/book3s_hv.c | 41 ++++++++++++++++++++++++++++++++++----
>> arch/powerpc/kvm/book3s_pr.c | 3 +++
>> arch/powerpc/kvm/emulate.c | 18 +++++++++++++++++
>> 5 files changed, 66 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
>> index fb86a22..dd83c9a 100644
>> --- a/arch/powerpc/include/asm/kvm_ppc.h
>> +++ b/arch/powerpc/include/asm/kvm_ppc.h
>> @@ -38,6 +38,12 @@
>> #include <asm/paca.h>
>> #endif
>>
>> +/*
>> + * KVMPPC_INST_SW_BREAKPOINT is debug Instruction
>> + * for supporting software breakpoint.
>> + */
>> +#define KVMPPC_INST_SW_BREAKPOINT 0x00dddd00
>> +
>> enum emulation_result {
>> EMULATE_DONE, /* no further processing */
>> EMULATE_DO_MMIO, /* kvm_run filled with MMIO request */
>> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
>> index dd03f6b..00e9c9f 100644
>> --- a/arch/powerpc/kvm/book3s.c
>> +++ b/arch/powerpc/kvm/book3s.c
>> @@ -778,7 +778,8 @@ int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu *vcpu,
>> int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>> struct kvm_guest_debug *dbg)
>> {
>> - return -EINVAL;
>> + vcpu->guest_debug = dbg->control;
>> + return 0;
>> }
>>
>> void kvmppc_decrementer_func(unsigned long data)
>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index 27cced9..3a2414c 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -725,6 +725,30 @@ static int kvmppc_hcall_impl_hv(unsigned long cmd)
>> return kvmppc_hcall_impl_hv_realmode(cmd);
>> }
>>
>> +static int kvmppc_emulate_debug_inst(struct kvm_run *run,
>> + struct kvm_vcpu *vcpu)
>> +{
>> + u32 last_inst;
>> +
>> + if(kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst) !=
>> + EMULATE_DONE) {
>> + /*
>> + * Fetch failed, so return to guest and
>> + * try executing it again.
>> + */
>> + return RESUME_GUEST;
>
> Please end the if() here. In the kernel we usually treat abort
> situations as separate.
>
OK. Will change it.
>> + } else {
>> + if (last_inst == KVMPPC_INST_SW_BREAKPOINT) {
>> + run->exit_reason = KVM_EXIT_DEBUG;
>> + run->debug.arch.address = kvmppc_get_pc(vcpu);
>> + return RESUME_HOST;
>> + } else {
>> + kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
>> + return RESUME_GUEST;
>> + }
>> + }
>> +}
>> +
>> static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
>> struct task_struct *tsk)
>> {
>> @@ -807,12 +831,18 @@ static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
>> break;
>> /*
>> * This occurs if the guest executes an illegal instruction.
>> - * We just generate a program interrupt to the guest, since
>> - * we don't emulate any guest instructions at this stage.
>> + * If the guest debug is disabled, generate a program interrupt
>> + * to the guest. If guest debug is enabled, we need to check
>> + * whether the instruction is a software breakpoint instruction.
>> + * Accordingly return to Guest or Host.
>> */
>> case BOOK3S_INTERRUPT_H_EMUL_ASSIST:
>> - kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
>> - r = RESUME_GUEST;
>> + if(vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP) {
>> + r = kvmppc_emulate_debug_inst(run, vcpu);
>> + } else {
>> + kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
>> + r = RESUME_GUEST;
>> + }
>> break;
>> /*
>> * This occurs if the guest (kernel or userspace), does something that
>> @@ -922,6 +952,9 @@ static int kvmppc_get_one_reg_hv(struct kvm_vcpu *vcpu, u64 id,
>> long int i;
>>
>> switch (id) {
>> + case KVM_REG_PPC_DEBUG_INST:
>> + *val = get_reg_val(id, KVMPPC_INST_SW_BREAKPOINT);
>> + break;
>> case KVM_REG_PPC_HIOR:
>> *val = get_reg_val(id, 0);
>> break;
>> diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
>> index faffb27..6d73708 100644
>> --- a/arch/powerpc/kvm/book3s_pr.c
>> +++ b/arch/powerpc/kvm/book3s_pr.c
>> @@ -1319,6 +1319,9 @@ static int kvmppc_get_one_reg_pr(struct kvm_vcpu *vcpu, u64 id,
>> int r = 0;
>>
>> switch (id) {
>> + case KVM_REG_PPC_DEBUG_INST:
>> + *val = get_reg_val(id, KVMPPC_INST_SW_BREAKPOINT);
>> + break;
>> case KVM_REG_PPC_HIOR:
>> *val = get_reg_val(id, to_book3s(vcpu)->hior);
>> break;
>> diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
>> index e96b50d..30f326b 100644
>> --- a/arch/powerpc/kvm/emulate.c
>> +++ b/arch/powerpc/kvm/emulate.c
>> @@ -274,6 +274,24 @@ int kvmppc_emulate_instruction(struct kvm_run *run, struct kvm_vcpu *vcpu)
>> }
>> break;
>>
>> + case 0:
>> + /*
>> + * Instruction with primary opcode 0. Based on PowerISA
>> + * these are illegal instructions.
>> + */
>> + if(inst == KVMPPC_INST_SW_BREAKPOINT) {
>
> Here you're missing a space between if and (. Are you sure that you ran
> checkpatch.pl?
>
No. my bad. Did not run checkpatch.pl. Will fix it.
>
>> + run->exit_reason = KVM_EXIT_DEBUG;
>> + run->debug.arch.address = kvmppc_get_pc(vcpu);
>> + emulated = EMULATE_EXIT_USER;
>> + advance = 0;
>> + printk(KERN_INFO "pr_emulate: debug instr \n");
>> + } else {
>> + kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
>> + emulated = EMULATE_DONE;
>> + advance = 0;
>
> Just set emulated = EMULATE_FAIL here, the same as default:.
>
>
Ok sure will do.
Thanks for review.
Maddy
> Alex
>
>> + }
>> + break;
>> +
>> default:
>> emulated = EMULATE_FAIL;
>> }
>>
>
^ permalink raw reply
* Re: bit fields && data tearing
From: H. Peter Anvin @ 2014-09-09 4:30 UTC (permalink / raw)
To: James Bottomley
Cc: Jakub Jelinek, One Thousand Gnomes, Tony Luck,
linux-ia64@vger.kernel.org, Peter Hurley, Mikael Pettersson,
Oleg Nesterov, linux-kernel@vger.kernel.org, Paul Mackerras,
linux-arch@vger.kernel.org, paulmck@linux.vnet.ibm.com,
linuxppc-dev@lists.ozlabs.org, Miroslav Franc, Richard Henderson
In-Reply-To: <1410231392.2028.15.camel@jarvis.lan>
Add a short member for proper alignment and one will probably pop out.
Sent from my tablet, pardon any formatting problems.
> On Sep 8, 2014, at 19:56, James Bottomley <James.Bottomley@HansenPartnersh=
ip.com> wrote:
>=20
>> On Mon, 2014-09-08 at 19:30 -0400, Peter Hurley wrote:
>> On 09/08/2014 01:50 AM, James Bottomley wrote:
>>>> Two things: I think that gcc has given up on combining adjacent writes,=
>>>> perhaps because unaligned writes on some arches are prohibitive, so
>>>> whatever minor optimization was believed to be gained was quickly lost,=
>>>> multi-fold. (Although this might not be the reason since one would
>>>> expect that gcc would know the alignment of the promoted store).
>>>=20
>>> Um, gcc assumes architecturally correct alignment; that's why it pads
>>> structures. Only when accessing packed structures will it use the
>>> lowest unit load/store.
>>>=20
>>> if you have a struct { char a, char b }; and load first a then b with a
>>> constant gcc will obligingly optimise to a short store.
>>=20
>> Um, please re-look at the test above. The exact test you describe is
>> coded above and compiled with gcc 4.6.3 cross-compiler for parisc using
>> the kernel compiler options.
>>=20
>> In the generated code, please note the _absence_ of a combined write
>> to two adjacent byte stores.
>=20
> So one version of gcc doesn't do it. Others do because I've been
> surprised seeing it in assembly.
>=20
>>>> But additionally, even if gcc combines adjacent writes _that are part
>>>> of the program flow_ then I believe the situation is no worse than
>>>> would otherwise exist.
>>>>=20
>>>> For instance, given the following:
>>>>=20
>>>> struct x {
>>>> spinlock_t lock;
>>>> long a;
>>>> byte b;
>>>> byte c;
>>>> };
>>>>=20
>>>> void locked_store_b(struct x *p)
>>>> {
>>>> spin_lock(&p->lock);
>>>> p->b =3D 1;
>>>> spin_unlock(&p->lock);
>>>> p->c =3D 2;
>>>> }
>>>>=20
>>>> Granted, the author probably expects ordered writes of
>>>> STORE B
>>>> STORE C
>>>> but that's not guaranteed because there is no memory barrier
>>>> ordering B before C.
>>>=20
>>> Yes, there is: loads and stores may not migrate into or out of critical
>>> sections.
>>=20
>> That's a common misconception.
>>=20
>> The processor is free to re-order this to:
>>=20
>> STORE C
>> STORE B
>> UNLOCK
>>=20
>> That's because the unlock() only guarantees that:
>>=20
>> Stores before the unlock in program order are guaranteed to complete
>> before the unlock completes. Stores after the unlock _may_ complete
>> before the unlock completes.
>>=20
>> My point was that even if compiler barriers had the same semantics
>> as memory barriers, the situation would be no worse. That is, code
>> that is sensitive to memory barriers (like the example I gave above)
>> would merely have the same fragility with one-way compiler barriers
>> (with respect to the compiler combining writes).
>>=20
>> That's what I meant by "no worse than would otherwise exist".
>=20
> Actually, that's not correct. This is actually deja vu with me on the
> other side of the argument. When we first did spinlocks on PA, I argued
> as you did: lock only a barrier for code after and unlock for code
> before. The failing case is that you can have a critical section which
> performs an atomically required operation and a following unit which
> depends on it being performed. If you begin the following unit before
> the atomic requirement, you may end up losing. It turns out this kind
> of pattern is inherent in a lot of mail box device drivers: you need to
> set up the mailbox atomically then poke it. Setup is usually atomic,
> deciding which mailbox to prime and actually poking it is in the
> following unit. Priming often involves an I/O bus transaction and if
> you poke before priming, you get a misfire.
>=20
>>>> I see no problem with gcc write-combining in the absence of
>>>> memory barriers (as long as alignment is still respected,
>>>> ie., the size-promoted write is still naturally aligned). The
>>>> combined write is still atomic.
>>>=20
>>> Actual alignment is pretty irrelevant. That's why all architectures
>>> which require alignment also have to implement misaligned traps ... this=
>>> is a fundamental requirement of the networking code, for instance.
>>>=20
>>> The main problem is gcc thinking there's a misalignment (or a packed
>>> structure). That causes splitting of loads and stores and that destroys=
>>> atomicity.
>>=20
>> Yeah, the extra requirement I added is basically nonsense, since the
>> only issue is what instructions the compiler is emitting. So if compiler
>> thinks the alignment is natural and combines the writes -- ok. If the
>> compiler thinks the alignment is off and doesn't combine the writes --
>> also ok.
>=20
> Yes, I think I can agree that the only real problem is gcc thinking the
> store or load needs splitting.
>=20
> James
>=20
>=20
^ permalink raw reply
* Re: bit fields && data tearing
From: H. Peter Anvin @ 2014-09-09 3:20 UTC (permalink / raw)
To: James Bottomley, Peter Hurley
Cc: Jakub Jelinek, One Thousand Gnomes, Tony Luck, linux-ia64,
Mikael Pettersson, Oleg Nesterov, linux-kernel, Paul Mackerras,
linux-arch, paulmck, linuxppc-dev, Miroslav Franc,
Richard Henderson
In-Reply-To: <1410231392.2028.15.camel@jarvis.lan>
On 09/08/2014 07:56 PM, James Bottomley wrote:
>>
>> Yeah, the extra requirement I added is basically nonsense, since the
>> only issue is what instructions the compiler is emitting. So if compiler
>> thinks the alignment is natural and combines the writes -- ok. If the
>> compiler thinks the alignment is off and doesn't combine the writes --
>> also ok.
>
> Yes, I think I can agree that the only real problem is gcc thinking the
> store or load needs splitting.
>
That seems much saner, and yes, that applies to any architecture which
needs unaligned references. Now, if the references are *actually*
unaligned they can end up being split even on x86, especially if they
cross cache line boundaries.
-hpa
^ permalink raw reply
* Re: bit fields && data tearing
From: James Bottomley @ 2014-09-09 2:56 UTC (permalink / raw)
To: Peter Hurley
Cc: Jakub Jelinek, One Thousand Gnomes, linux-arch, linux-ia64,
Mikael Pettersson, Oleg Nesterov, linux-kernel, Tony Luck,
Paul Mackerras, H. Peter Anvin, paulmck, linuxppc-dev,
Miroslav Franc, Richard Henderson
In-Reply-To: <540E3BFF.7080307@hurleysoftware.com>
On Mon, 2014-09-08 at 19:30 -0400, Peter Hurley wrote:
> On 09/08/2014 01:50 AM, James Bottomley wrote:
> >> Two things: I think that gcc has given up on combining adjacent writes,
> >> perhaps because unaligned writes on some arches are prohibitive, so
> >> whatever minor optimization was believed to be gained was quickly lost,
> >> multi-fold. (Although this might not be the reason since one would
> >> expect that gcc would know the alignment of the promoted store).
> >
> > Um, gcc assumes architecturally correct alignment; that's why it pads
> > structures. Only when accessing packed structures will it use the
> > lowest unit load/store.
> >
> > if you have a struct { char a, char b }; and load first a then b with a
> > constant gcc will obligingly optimise to a short store.
>
> Um, please re-look at the test above. The exact test you describe is
> coded above and compiled with gcc 4.6.3 cross-compiler for parisc using
> the kernel compiler options.
>
> In the generated code, please note the _absence_ of a combined write
> to two adjacent byte stores.
So one version of gcc doesn't do it. Others do because I've been
surprised seeing it in assembly.
> >> But additionally, even if gcc combines adjacent writes _that are part
> >> of the program flow_ then I believe the situation is no worse than
> >> would otherwise exist.
> >>
> >> For instance, given the following:
> >>
> >> struct x {
> >> spinlock_t lock;
> >> long a;
> >> byte b;
> >> byte c;
> >> };
> >>
> >> void locked_store_b(struct x *p)
> >> {
> >> spin_lock(&p->lock);
> >> p->b = 1;
> >> spin_unlock(&p->lock);
> >> p->c = 2;
> >> }
> >>
> >> Granted, the author probably expects ordered writes of
> >> STORE B
> >> STORE C
> >> but that's not guaranteed because there is no memory barrier
> >> ordering B before C.
> >
> > Yes, there is: loads and stores may not migrate into or out of critical
> > sections.
>
> That's a common misconception.
>
> The processor is free to re-order this to:
>
> STORE C
> STORE B
> UNLOCK
>
> That's because the unlock() only guarantees that:
>
> Stores before the unlock in program order are guaranteed to complete
> before the unlock completes. Stores after the unlock _may_ complete
> before the unlock completes.
>
> My point was that even if compiler barriers had the same semantics
> as memory barriers, the situation would be no worse. That is, code
> that is sensitive to memory barriers (like the example I gave above)
> would merely have the same fragility with one-way compiler barriers
> (with respect to the compiler combining writes).
>
> That's what I meant by "no worse than would otherwise exist".
Actually, that's not correct. This is actually deja vu with me on the
other side of the argument. When we first did spinlocks on PA, I argued
as you did: lock only a barrier for code after and unlock for code
before. The failing case is that you can have a critical section which
performs an atomically required operation and a following unit which
depends on it being performed. If you begin the following unit before
the atomic requirement, you may end up losing. It turns out this kind
of pattern is inherent in a lot of mail box device drivers: you need to
set up the mailbox atomically then poke it. Setup is usually atomic,
deciding which mailbox to prime and actually poking it is in the
following unit. Priming often involves an I/O bus transaction and if
you poke before priming, you get a misfire.
> >> I see no problem with gcc write-combining in the absence of
> >> memory barriers (as long as alignment is still respected,
> >> ie., the size-promoted write is still naturally aligned). The
> >> combined write is still atomic.
> >
> > Actual alignment is pretty irrelevant. That's why all architectures
> > which require alignment also have to implement misaligned traps ... this
> > is a fundamental requirement of the networking code, for instance.
> >
> > The main problem is gcc thinking there's a misalignment (or a packed
> > structure). That causes splitting of loads and stores and that destroys
> > atomicity.
>
> Yeah, the extra requirement I added is basically nonsense, since the
> only issue is what instructions the compiler is emitting. So if compiler
> thinks the alignment is natural and combines the writes -- ok. If the
> compiler thinks the alignment is off and doesn't combine the writes --
> also ok.
Yes, I think I can agree that the only real problem is gcc thinking the
store or load needs splitting.
James
^ permalink raw reply
* Re: bit fields && data tearing
From: H. Peter Anvin @ 2014-09-09 2:30 UTC (permalink / raw)
To: James Bottomley
Cc: Jakub Jelinek, One Thousand Gnomes, Tony Luck, linux-ia64,
Peter Hurley, Mikael Pettersson, Oleg Nesterov, linux-kernel,
Paul Mackerras, linux-arch, paulmck, linuxppc-dev, Miroslav Franc,
Richard Henderson
In-Reply-To: <1410215994.2027.86.camel@jarvis.lan>
On 09/08/2014 03:39 PM, James Bottomley wrote:
>
> I don't understand what you mean by "pass each other". Atomicity
> guarantees are not ordering guarantees in a SMP environment. The
> guarantee is that if you follow the rules when two CPUs update the same
> natural width aligned object simultaneously using the same primitive,
> the result is either one or the other of their updates. Which one wins
> (the ordering) isn't defined.
>
I'm trying to figure out why it would possibly make a difference in any
kind of sane system if gcc fuses accesses.
Assuming bigendian for the moment, I would expect that if CPU 1 does a
write of 0x01020304 to address 0 and CPU 2 does a write of 0x0506 to
address 2, that the end result would be either 0x01020304 or 0x01020506.
Similarly, I would expect that if these operations are both done on the
same CPU in that order, that the result would unambiguously be 0x01020506.
I would strongly suspect an architecture which does not provide those
guarantees is an outlier.
-hpa
^ permalink raw reply
* Re: bit fields && data tearing
From: H. Peter Anvin @ 2014-09-09 2:27 UTC (permalink / raw)
To: James Bottomley, Chris Metcalf
Cc: Jakub Jelinek, One Thousand Gnomes, Tony Luck, linux-ia64,
Peter Hurley, Mikael Pettersson, Oleg Nesterov, linux-kernel,
Paul Mackerras, linux-arch, paulmck, linuxppc-dev, Miroslav Franc,
Richard Henderson
In-Reply-To: <1410216228.2027.88.camel@jarvis.lan>
On 09/08/2014 03:43 PM, James Bottomley wrote:
>
> This was years ago (possibly decades). We had to implement in-kernel
> unaligned traps for the networking layer because it could access short
> and int fields that weren't of the correct alignment when processing
> packets. It that's all corrected now, we wouldn't really notice (except
> a bit of a speed up since an unaligned trap effectively places the
> broken out instructions into the bit stream).
>
> James
>
Well, ARM doesn't trap, it just silently gives garbage on unaligned
memory references.
-hpa
^ permalink raw reply
* Re: [Xen-devel] [PATCH v1 08/21] x86/xen/MSI: Use MSI chip framework to configure MSI/MSI-X irq
From: Yijing Wang @ 2014-09-09 2:06 UTC (permalink / raw)
To: David Vrabel, Bjorn Helgaas
Cc: linux-mips, linux-ia64, linux-pci, Xinwei Hu, sparclinux,
linux-arch, linux-s390, Russell King, Joerg Roedel, x86,
Sebastian Ott, xen-devel, arnab.basu, Arnd Bergmann,
Chris Metcalf, Thomas Gleixner, linux-arm-kernel, Bharat.Bhushan,
Tony Luck, Ralf Baechle, iommu, Wuyun, linuxppc-dev,
David S. Miller
In-Reply-To: <5409C8C0.8020200@citrix.com>
On 2014/9/5 22:29, David Vrabel wrote:
> On 05/09/14 11:09, Yijing Wang wrote:
>> Use MSI chip framework instead of arch MSI functions to configure
>> MSI/MSI-X irq. So we can manage MSI/MSI-X irq in a unified framework.
> [...]
>> --- a/arch/x86/pci/xen.c
>> +++ b/arch/x86/pci/xen.c
> [...]
>> @@ -418,9 +430,9 @@ int __init pci_xen_init(void)
>> #endif
>>
>> #ifdef CONFIG_PCI_MSI
>> - x86_msi.setup_msi_irqs = xen_setup_msi_irqs;
>> - x86_msi.teardown_msi_irq = xen_teardown_msi_irq;
>> - x86_msi.teardown_msi_irqs = xen_teardown_msi_irqs;
>> + xen_msi_chip.setup_irqs = xen_setup_msi_irqs;
>> + xen_msi_chip.teardown_irqs = xen_teardown_msi_irqs;
>> + x86_msi_chip = &xen_msi_chip;
>> msi_chip.irq_mask = xen_nop_msi_mask;
>> msi_chip.irq_unmask = xen_nop_msi_mask;
>
> Why have these not been changed to set the x86_msi_chip.mask/unmask
> fields instead?
Hi David, x86_msi_chip here is struct msi_chip data type, used to configure MSI/MSI-X
irq. msi_chip above is struct irq_chip data type, represent the MSI irq controller. They are
not the same object. Their name easily confusing people.
Defined in arch/x86/kernel/apic/io_apic.c
/*
* IRQ Chip for MSI PCI/PCI-X/PCI-Express Devices,
* which implement the MSI or MSI-X Capability Structure.
*/
static struct irq_chip msi_chip = {
.name = "PCI-MSI",
.irq_unmask = unmask_msi_irq,
.irq_mask = mask_msi_irq,
.irq_ack = ack_apic_edge,
.irq_set_affinity = msi_set_affinity,
.irq_retrigger = ioapic_retrigger_irq,
};
Defined in arch/x86/kernel/apic/io_apic.c, introduced in patch 7/21
struct msi_chip apic_msi_chip = {
.setup_irqs = native_setup_msi_irqs,
.teardown_irq = native_teardown_msi_irq,
};
[...]
struct msi_chip *x86_msi_chip = &apic_msi_chip;
Thanks!
Yijing.
>
> David
>
> .
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: bit fields && data tearing
From: Paul E. McKenney @ 2014-09-09 1:59 UTC (permalink / raw)
To: Peter Hurley
Cc: Jakub Jelinek, One Thousand Gnomes, Tony Luck,
linux-ia64@vger.kernel.org, H. Peter Anvin, Oleg Nesterov,
linux-kernel@vger.kernel.org, linux-alpha, David Laight,
Paul Mackerras, linux-arch@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, Miroslav Franc, Richard Henderson
In-Reply-To: <540E3207.7090007@hurleysoftware.com>
On Mon, Sep 08, 2014 at 06:47:35PM -0400, Peter Hurley wrote:
> On 09/08/2014 01:59 PM, H. Peter Anvin wrote:
> > On 09/08/2014 10:52 AM, One Thousand Gnomes wrote:
> >> On Fri, 05 Sep 2014 08:41:52 -0700
> >> "H. Peter Anvin" <hpa@zytor.com> wrote:
> >>
> >>> On 09/05/2014 08:31 AM, Peter Hurley wrote:
> >>>>
> >>>> Which is a bit ironic because I remember when Digital had a team
> >>>> working on emulating native x86 apps on Alpha/NT.
> >>>>
> >>>
> >>> Right, because the x86 architecture was obsolete and would never scale...
> >>
> >> Talking about "not scaling" can anyone explain how a "you need to use
> >> set_bit() and friends" bug report scaled into a hundred message plus
> >> discussion about ambiguous properties of processors (and nobody has
> >> audited all the embedded platforms we support yet, or the weirder ARMs)
> >> and a propsal to remove Alpha support.
> >>
> >> Wouldn't it be *much* simpler to do what I suggested in the first place
> >> and use the existing intended for purpose, deliberately put there,
> >> functions for atomic bitops, because they are fast on sane processors and
> >> they work on everything else.
> >>
> >> I think the whole "removing Alpha EV5" support is basically bonkers. Just
> >> use set_bit in the tty layer. Alpha will continue to work as well as it
> >> always has done and you won't design out support for any future processor
> >> that turns out not to do byte aligned stores.
> >>
> >> Alan
> >>
> >
> > Is *that* what we are talking about? I was added to this conversation
> > in the middle where it had already generalized, so I had no idea.
>
> No, this is just what brought this craziness to my attention.
>
> For example, byte- and short-sized circular buffers could not possibly
> be safe either, when the head nears the tail.
>
> Who has audited global storage and ensured that _every_ byte-sized write
> doesn't happen to be adjacent to some other storage that may not happen
> to be protected by the same (or any) lock?
This was my concern as well.
Thanx, Paul
^ permalink raw reply
* Re: [PATCH] powerpc: Add macros for the ibm_architecture_vec[] lengths
From: Stewart Smith @ 2014-09-09 1:03 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev
In-Reply-To: <1409295703-31174-1-git-send-email-mpe@ellerman.id.au>
Michael Ellerman <mpe@ellerman.id.au> writes:
> The encoding of the lengths in the ibm_architecture_vec array is
> "interesting" to say the least. It's non-obvious how the number of bytes
> we provide relates to the length value.
>
> In fact we already got it wrong once, see 11e9ed43ca8a "Fix up
> ibm_architecture_vec definition".
>
> So add some macros to make it (hopefully) clearer. These at least have
> the property that the integer present in the code is equal to the number
> of bytes that follows it.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Seems at least as correct as the code was before... so,
Reviewed-by: Stewart Smith <stewart@linux.vnet.ibm.com>
(not actually compiled or tested or anything, but my internal C
preprocesser says it looks okay :)
^ permalink raw reply
* Re: [PATCH 0/2] PCI/MSI: Remove arch_msi_check_device()
From: Michael Ellerman @ 2014-09-09 0:43 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci@vger.kernel.org, Alexander Gordeev, linuxppc-dev,
linux-kernel@vger.kernel.org
In-Reply-To: <CAErSpo4XN01hgTUpcca=i-=LFV_bM-Cok8614LJzrOfVbx8MAA@mail.gmail.com>
On Fri, 2014-09-05 at 15:27 -0600, Bjorn Helgaas wrote:
> On Fri, Sep 5, 2014 at 3:25 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > On Sat, Jul 12, 2014 at 01:21:06PM +0200, Alexander Gordeev wrote:
> >> Hello,
> >>
> >> This is a cleanup effort to get rid of useless arch_msi_check_device().
> >> I am not sure what were the reasons for its existence in the first place,
> >> but at the moment it appears totally unnecessary.
> >>
> >> Thanks!
> >>
> >> Cc: linuxppc-dev@lists.ozlabs.org
> >> Cc: linux-pci@vger.kernel.org
> >>
> >> Alexander Gordeev (2):
> >> PCI/MSI/PPC: Remove arch_msi_check_device()
> >> PCI/MSI: Remove arch_msi_check_device()
> >
> > I applied these (with Michael's ack on the first, and v2 of the second) to
> > pci/msi for v3.18, thanks!
>
> Oh, I forgot -- if you'd rather take the first one through the PPC
> tree, you can do that and I can merge the second one later. Let me
> know if you want to do that.
You take them all, that code shouldn't be changing much in the powerpc tree.
If the merge gets messy closer to the merge window we can always take the first
patch into the powerpc tree then.
cheers
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox