* [PATCH] mfd: qcom-pm8xxx: Clean up PM8XXX namespace
From: Lee Jones @ 2016-11-09 15:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477487453-15801-1-git-send-email-linus.walleij@linaro.org>
On Wed, 26 Oct 2016, Linus Walleij wrote:
> The Kconfig and file naming for the PM8xxx driver is totally
> confusing:
>
> - Kconfig options MFD_PM8XXX and MFD_PM8921_CORE, some in-kernel
> users depending on or selecting either at random.
> - A driver file named pm8921-core.c even if it is indeed
> used by the whole PM8xxx family of chips.
> - An irqchip named pm8xxx since it was (I guess) realized that
> the driver was generic for all pm8xxx PMICs.
>
> As I may want to add support for PM8901 this is starting to get
> really messy. Fix this situation by:
>
> - Remove the MFD_PM8921_CORE symbol and rely solely on MFD_PM8XXX
> and convert all users, including LEDs Kconfig and ARM defconfigs
> for qcom and multi_v7 to use that single symbol.
> - Renaming the driver to qcom-pm8xxx.c to fit along the two
> other qcom* prefixed drivers.
> - Rename functions withing the driver from 8921 to 8xxx to
> indicate it is generic.
> - Just drop the =m config from the pxa_defconfig, I have no clue
> why it is even there, it is not a Qualcomm platform. (Possibly
> older Kconfig noise from saveconfig.)
>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> I do NOT think it is a good idea to try to split this commit up,
> I rather prefer that Lee simply merge it into MFD.
>
> The reason is that files like qcom_defconfig already contain both
> the right symbols, but the MFD_PM8921_CORE symbol cannot be removed
> until this rename has happened, whereas multi_v7_defconfig needs
> it added etc, and this is just a clean nice cut.
>
> Jacek, ARM SoC person: please ACK this patch to get merged into
> MFD.
> ---
> arch/arm/configs/multi_v7_defconfig | 2 +-
> arch/arm/configs/pxa_defconfig | 1 -
> arch/arm/configs/qcom_defconfig | 1 -
> drivers/leds/Kconfig | 2 +-
> drivers/mfd/Kconfig | 14 ++++------
> drivers/mfd/Makefile | 2 +-
> drivers/mfd/{pm8921-core.c => qcom-pm8xxx.c} | 42 ++++++++++++++--------------
For my own reference:
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
> 7 files changed, 29 insertions(+), 35 deletions(-)
> rename drivers/mfd/{pm8921-core.c => qcom-pm8xxx.c} (92%)
How many more Acks do we need?
[...]
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH 2/3] ipmi/bt-bmc: maintain a request expiry list
From: Corey Minyard @ 2016-11-09 15:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <6117c1fd-b969-9394-0be5-d46f64269cac@kaod.org>
On 11/09/2016 08:30 AM, C?dric Le Goater wrote:
> On 11/07/2016 08:04 PM, Corey Minyard wrote:
>> On 11/02/2016 02:57 AM, C?dric Le Goater wrote:
>>> Regarding the response expiration handling, the IPMI spec says :
>>>
>>> The BMC must not return a given response once the corresponding
>>> Request-to-Response interval has passed. The BMC can ensure this
>>> by maintaining its own internal list of outstanding requests through
>>> the interface. The BMC could age and expire the entries in the list
>>> by expiring the entries at an interval that is somewhat shorter than
>>> the specified Request-to-Response interval....
>>>
>>> To handle such case, we maintain list of received requests using the
>>> seq number of the BT message to identify them. The list is updated
>>> each time a request is received and a response is sent. The expiration
>>> of the reponses is handled at each updates but also with a timer.
>> This looks correct, but it seems awfully complicated.
>>
>> Why can't you get the current time before the wait_event_interruptible()
>> and then compare the time before you do the write? That would seem to
>> accomplish the same thing without any lists or extra locks.
> Well, the expiry list needs a request identifier and it is currently using
> the Seq byte for this purpose. So the BT message needs to be read to grab
> that byte. The request is added to a list and that involves some locking.
>
> When the response is written, the first matching request is removed from
> the list and a garbage collector loop is also run. Then, as we might not
> get any responses to run that loop, we use a timer to empty the list from
> any expired requests.
>
> The read/write ops of the driver are protected with a mutex, the list and
> the timer add their share of locking. That could have been done with RCU
> surely but we don't really need performance in this driver.
>
> Caveats :
>
> bt_bmc_remove_request() should not be done in the writing loop though.
> It needs a fix.
>
> The request identifier is currently Seq but the spec say we should use
> Seq, NetFn, and Command or an internal Seq value as a request identifier.
> Google is also working on an OEM/Group extension (Brendan in CC: )
> which has a different message format. I need to look closer at what
> should be done in this case.
I'm still not sure why the list is necessary. You have a separate
thread of execution for each writer, why not just time it in that
thread?
What about the following, not even compile-tested, patch? I'm
sure my mailer will munge this up, I can send you a clean version
if you like.
From 1a73585a9c1c74ac1d59d82f22e05b30447619a6 Mon Sep 17 00:00:00 2001
From: Corey Minyard <cminyard@mvista.com>
Date: Wed, 9 Nov 2016 09:07:48 -0600
Subject: [PATCH] ipmi:bt-bmc: Fix a multi-user race, time out responses
The IPMI spec says to time out responses after a given amount of
time, so don't let a writer send something after an amount of time
has elapsed.
Also, fix a race condition in the same area where if you have two
writers at the same time, one can get a EIO return when it should
still be waiting its turn to send. A mutex_lock_interruptible_timeout()
would be handy here, but it doesn't exist.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
drivers/char/ipmi/bt-bmc.c | 39 ++++++++++++++++++++++++---------------
1 file changed, 24 insertions(+), 15 deletions(-)
diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c
index b49e613..5be94cf 100644
--- a/drivers/char/ipmi/bt-bmc.c
+++ b/drivers/char/ipmi/bt-bmc.c
@@ -57,6 +57,8 @@
#define BT_BMC_BUFFER_SIZE 256
+#define BT_BMC_RESPONSE_JIFFIES (5 * HZ)
+
struct bt_bmc {
struct device dev;
struct miscdevice miscdev;
@@ -190,14 +192,12 @@ static ssize_t bt_bmc_read(struct file *file, char
__user *buf,
WARN_ON(*ppos);
- if (wait_event_interruptible(bt_bmc->queue,
- bt_inb(bt_bmc, BT_CTRL) & BT_CTRL_H2B_ATN))
+ if (mutex_lock_interruptible(&bt_bmc->mutex))
return -ERESTARTSYS;
- mutex_lock(&bt_bmc->mutex);
-
- if (unlikely(!(bt_inb(bt_bmc, BT_CTRL) & BT_CTRL_H2B_ATN))) {
- ret = -EIO;
+ if (wait_event_interruptible(bt_bmc->queue,
+ bt_inb(bt_bmc, BT_CTRL) & BT_CTRL_H2B_ATN)) {
+ ret = -ERESTARTSYS;
goto out_unlock;
}
@@ -251,6 +251,7 @@ static ssize_t bt_bmc_write(struct file *file, const
char __user *buf,
u8 kbuffer[BT_BMC_BUFFER_SIZE];
ssize_t ret = 0;
ssize_t nwritten;
+ unsigned long start_jiffies = jiffies, wait_time;
/*
* send a minimum response size
@@ -263,23 +264,31 @@ static ssize_t bt_bmc_write(struct file *file,
const char __user *buf,
WARN_ON(*ppos);
+ if (mutex_lock_interruptible(&bt_bmc->mutex))
+ return -ERESTARTSYS;
+
+ wait_time = jiffies - start_jiffies;
+ if (wait_time >= BT_BMC_RESPONSE_TIME_JIFFIES) {
+ ret = -ETIMEDOUT;
+ goto out_unlock;
+ }
+ wait_time = BT_BMC_RESPONSE_TIME_JIFFIES - wait_time;
+
/*
* There's no interrupt for clearing bmc busy so we have to
* poll
*/
- if (wait_event_interruptible(bt_bmc->queue,
+ ret = wait_event_interruptible_timeout(bt_bmc->queue,
!(bt_inb(bt_bmc, BT_CTRL) &
- (BT_CTRL_H_BUSY | BT_CTRL_B2H_ATN))))
- return -ERESTARTSYS;
-
- mutex_lock(&bt_bmc->mutex);
-
- if (unlikely(bt_inb(bt_bmc, BT_CTRL) &
- (BT_CTRL_H_BUSY | BT_CTRL_B2H_ATN))) {
- ret = -EIO;
+ (BT_CTRL_H_BUSY | BT_CTRL_B2H_ATN)),
+ wait_time);
+ if (ret <= 0) {
+ if (ret == 0)
+ ret = -ETIMEDOUT;
goto out_unlock;
}
+ ret = 0;
clr_wr_ptr(bt_bmc);
while (count) {
--
2.7.4
>> Also, if you are going to have multiple writers on this interface, I don't
>> think the interface will work correctly. I think you need to claim the
>> mutex first. Otherwise you might get into a situation where two writers
>> will wake up at the same time, the first writes and releases the mutex,
>> then the second will find that the interface is busy and fail.
> yes. that is a current problem in the driver and it is not really an
> elegant way to handle concurrency. We are fine for the moment as we
> only have one single threaded process using the device.
>
>> If I am correct, the mutex will need to become interruptible and come
>> first, I think. (And the timing would have to start before the mutex,
>> of course.) This applies to both the read and write interface.
> OK. I will look into fixing this problem first.
>
>> Another thing is that this is request-to-release time. If a request takes
>> a long time to process (say, a write to a flash device) the timeout would
>> need to be decreased by the processing time.
> Hmm, how would that fit with the "BT Interface Capabilities" which
> defines :
>
> BMC Request-to-Response time, in seconds, 1 based. 30 seconds, maximum.
>
> This is a fixed value. And the spec only say :
>
> The BMC could age and expire the entries in the list by expiring
> the entries at an interval that is somewhat shorter than the
> specified Request-to-Response interval.
>
> May be I am misunderstanding.
Yeah, as usual, the IPMI spec is kind of vague about this. You have
to think about the problem from the host driver's point of view to
know what this means.
From a host driver point of view, it's going to send a request and wait
for a response with the same sequence number. It should time out the
request in "BMC Request-to-Response time" seconds. After that point,
it's free to re-use the sequence number. So what a BMC cannot do is
send that response after the timeout.
If the timeout is 5 seconds, and the BMC gets a flash write request that
takes 3 seconds then sits waiting for 2 seconds, it should not send the
response because the host driver may mistake it for a request it just
sent. So the timeout should be based on when the BMC got the request,
not when the response was queued for write, and that's the reason that
the BMC timer should be somewhat shorter than the host timer, as it
says.
I don't see an easy way to do this. Some possibilities I can think of are:
* Switch to using an ioctl to do the read/write operation. That's
kind of ugly. It's what the IPMI driver interface does, and it
has been somewhat of a pain.
* You could define a "header" that is put into the read message
and write message. You could add an ioctl to query the header
size; if the ioctl fails then the driver doesn't have a header.
Add a jiffie to the header of the read message that the
BMC userland must put into the header of the write message.
You could even have the ioctl enable the header, to preserve
backwards compatibility.
* Define some sort of structure that you read/write. This has all
sorts of issues.
I think my preference is the second, it allows for backwards
compatibility and lets the kernel do basically whatever it wants with
the data.
>> It's probably ok to not do that for the moment, but you may want to add
>> a note. Fixing that would require adding a timeout for each message.
> Thanks,
>
> C.
>
>> -corey
>>
>>
>>> Signed-off-by: C?dric Le Goater <clg@kaod.org>
>>> ---
>>> drivers/char/ipmi/bt-bmc.c | 132 +++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 132 insertions(+)
>>>
>>> diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c
>>> index fc9e8891eae3..e751e4a754b7 100644
>>> --- a/drivers/char/ipmi/bt-bmc.c
>>> +++ b/drivers/char/ipmi/bt-bmc.c
>>> @@ -17,6 +17,7 @@
>>> #include <linux/platform_device.h>
>>> #include <linux/poll.h>
>>> #include <linux/sched.h>
>>> +#include <linux/slab.h>
>>> #include <linux/timer.h>
>>> /*
>>> @@ -57,6 +58,15 @@
>>> #define BT_BMC_BUFFER_SIZE 256
>>> +#define BT_BMC_RESPONSE_TIME 5 /* seconds */
>>> +
>>> +struct ipmi_request {
>>> + struct list_head list;
>>> +
>>> + u8 seq;
>>> + unsigned long expires;
>>> +};
>>> +
>>> struct bt_bmc {
>>> struct device dev;
>>> struct miscdevice miscdev;
>>> @@ -65,6 +75,11 @@ struct bt_bmc {
>>> wait_queue_head_t queue;
>>> struct timer_list poll_timer;
>>> struct mutex mutex;
>>> +
>>> + unsigned int response_time;
>>> + struct timer_list requests_timer;
>>> + spinlock_t requests_lock;
>>> + struct list_head requests;
>>> };
>>> static atomic_t open_count = ATOMIC_INIT(0);
>>> @@ -163,6 +178,94 @@ static int bt_bmc_open(struct inode *inode, struct file *file)
>>> }
>>> /*
>>> + * lock should be held
>>> + */
>>> +static void drop_expired_requests(struct bt_bmc *bt_bmc)
>>> +{
>>> + unsigned long flags = 0;
>>> + struct ipmi_request *req, *next;
>>> + unsigned long next_expires = 0;
>>> + int start_timer = 0;
>>> +
>>> + spin_lock_irqsave(&bt_bmc->requests_lock, flags);
>>> + list_for_each_entry_safe(req, next, &bt_bmc->requests, list) {
>>> + if (time_after_eq(jiffies, req->expires)) {
>>> + pr_warn("BT: request seq:%d has expired. dropping\n",
>>> + req->seq);
>>> + list_del(&req->list);
>>> + kfree(req);
>>> + continue;
>>> + }
>>> + if (!start_timer) {
>>> + start_timer = 1;
>>> + next_expires = req->expires;
>>> + } else {
>>> + next_expires = min(next_expires, req->expires);
>>> + }
>>> + }
>>> + spin_unlock_irqrestore(&bt_bmc->requests_lock, flags);
>>> +
>>> + /* and possibly restart */
>>> + if (start_timer)
>>> + mod_timer(&bt_bmc->requests_timer, next_expires);
>>> +}
>>> +
>>> +static void requests_timer(unsigned long data)
>>> +{
>>> + struct bt_bmc *bt_bmc = (void *)data;
>>> +
>>> + drop_expired_requests(bt_bmc);
>>> +}
>>> +
>>> +static int bt_bmc_add_request(struct bt_bmc *bt_bmc, u8 seq)
>>> +{
>>> + struct ipmi_request *req;
>>> +
>>> + req = kmalloc(sizeof(*req), GFP_KERNEL);
>>> + if (!req)
>>> + return -ENOMEM;
>>> +
>>> + req->seq = seq;
>>> + req->expires = jiffies +
>>> + msecs_to_jiffies(bt_bmc->response_time * 1000);
>>> +
>>> + spin_lock(&bt_bmc->requests_lock);
>>> + list_add(&req->list, &bt_bmc->requests);
>>> + spin_unlock(&bt_bmc->requests_lock);
>>> +
>>> + drop_expired_requests(bt_bmc);
>>> + return 0;
>>> +}
>>> +
>>> +static int bt_bmc_remove_request(struct bt_bmc *bt_bmc, u8 seq)
>>> +{
>>> + struct ipmi_request *req, *next;
>>> + int ret = -EBADRQC; /* Invalid request code */
>>> +
>>> + spin_lock(&bt_bmc->requests_lock);
>>> + list_for_each_entry_safe(req, next, &bt_bmc->requests, list) {
>>> + /*
>>> + * The sequence number should be unique, so remove the
>>> + * first matching request found. If there are others,
>>> + * they should expire
>>> + */
>>> + if (req->seq == seq) {
>>> + list_del(&req->list);
>>> + kfree(req);
>>> + ret = 0;
>>> + break;
>>> + }
>>> + }
>>> + spin_unlock(&bt_bmc->requests_lock);
>>> +
>>> + if (ret)
>>> + pr_warn("BT: request seq:%d is invalid\n", seq);
>>> +
>>> + drop_expired_requests(bt_bmc);
>>> + return ret;
>>> +}
>>> +
>>> +/*
>>> * The BT (Block Transfer) interface means that entire messages are
>>> * buffered by the host before a notification is sent to the BMC that
>>> * there is data to be read. The first byte is the length and the
>>> @@ -231,6 +334,13 @@ static ssize_t bt_bmc_read(struct file *file, char __user *buf,
>>> len_byte = 0;
>>> }
>>> + if (ret > 0) {
>>> + int ret2 = bt_bmc_add_request(bt_bmc, kbuffer[2]);
>>> +
>>> + if (ret2)
>>> + ret = ret2;
>>> + }
>>> +
>>> clr_b_busy(bt_bmc);
>>> out_unlock:
>>> @@ -283,12 +393,20 @@ static ssize_t bt_bmc_write(struct file *file, const char __user *buf,
>>> clr_wr_ptr(bt_bmc);
>>> while (count) {
>>> + int ret2;
>>> +
>>> nwritten = min_t(ssize_t, count, sizeof(kbuffer));
>>> if (copy_from_user(&kbuffer, buf, nwritten)) {
>>> ret = -EFAULT;
>>> break;
>>> }
>>> + ret2 = bt_bmc_remove_request(bt_bmc, kbuffer[2]);
>>> + if (ret2) {
>>> + ret = ret2;
>>> + break;
>>> + }
>>> +
>>> bt_writen(bt_bmc, kbuffer, nwritten);
>>> count -= nwritten;
>>> @@ -438,6 +556,8 @@ static int bt_bmc_probe(struct platform_device *pdev)
>>> mutex_init(&bt_bmc->mutex);
>>> init_waitqueue_head(&bt_bmc->queue);
>>> + INIT_LIST_HEAD(&bt_bmc->requests);
>>> + spin_lock_init(&bt_bmc->requests_lock);
>>> bt_bmc->miscdev.minor = MISC_DYNAMIC_MINOR,
>>> bt_bmc->miscdev.name = DEVICE_NAME,
>>> @@ -451,6 +571,8 @@ static int bt_bmc_probe(struct platform_device *pdev)
>>> bt_bmc_config_irq(bt_bmc, pdev);
>>> + bt_bmc->response_time = BT_BMC_RESPONSE_TIME;
>>> +
>>> if (bt_bmc->irq) {
>>> dev_info(dev, "Using IRQ %d\n", bt_bmc->irq);
>>> } else {
>>> @@ -468,6 +590,9 @@ static int bt_bmc_probe(struct platform_device *pdev)
>>> BT_CR0_ENABLE_IBT,
>>> bt_bmc->base + BT_CR0);
>>> + setup_timer(&bt_bmc->requests_timer, requests_timer,
>>> + (unsigned long)bt_bmc);
>>> +
>>> clr_b_busy(bt_bmc);
>>> return 0;
>>> @@ -476,10 +601,17 @@ static int bt_bmc_probe(struct platform_device *pdev)
>>> static int bt_bmc_remove(struct platform_device *pdev)
>>> {
>>> struct bt_bmc *bt_bmc = dev_get_drvdata(&pdev->dev);
>>> + struct ipmi_request *req, *next;
>>> misc_deregister(&bt_bmc->miscdev);
>>> if (!bt_bmc->irq)
>>> del_timer_sync(&bt_bmc->poll_timer);
>>> +
>>> + del_timer_sync(&bt_bmc->requests_timer);
>>> + list_for_each_entry_safe(req, next, &bt_bmc->requests, list) {
>>> + list_del(&req->list);
>>> + kfree(req);
>>> + }
>>> return 0;
>>> }
>>>
>>
^ permalink raw reply related
* [PATCH v2 2/2] mm: hugetlb: support gigantic surplus pages
From: Gerald Schaefer @ 2016-11-09 15:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478675294-2507-1-git-send-email-shijie.huang@arm.com>
On Wed, 9 Nov 2016 15:08:14 +0800
Huang Shijie <shijie.huang@arm.com> wrote:
> When testing the gigantic page whose order is too large for the buddy
> allocator, the libhugetlbfs test case "counter.sh" will fail.
>
> The failure is caused by:
> 1) kernel fails to allocate a gigantic page for the surplus case.
> And the gather_surplus_pages() will return NULL in the end.
>
> 2) The condition checks for "over-commit" is wrong.
>
> This patch adds code to allocate the gigantic page in the
> __alloc_huge_page(). After this patch, gather_surplus_pages()
> can return a gigantic page for the surplus case.
>
> This patch changes the condition checks for:
> return_unused_surplus_pages()
> nr_overcommit_hugepages_store()
> hugetlb_overcommit_handler()
>
> This patch also set @nid with proper value when NUMA_NO_NODE is
> passed to alloc_gigantic_page().
>
> After this patch, the counter.sh can pass for the gigantic page.
>
> Acked-by: Steve Capper <steve.capper@arm.com>
> Signed-off-by: Huang Shijie <shijie.huang@arm.com>
> ---
> 1. fix the wrong check in hugetlb_overcommit_handler();
> 2. try to fix the s390 issue.
> ---
> mm/hugetlb.c | 20 ++++++++++++++------
> 1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 9fdfc24..5dbfd62 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1095,6 +1095,9 @@ static struct page *alloc_gigantic_page(int nid, unsigned int order)
> unsigned long ret, pfn, flags;
> struct zone *z;
>
> + if (nid == NUMA_NO_NODE)
> + nid = numa_mem_id();
> +
Now counter.sh works (on s390) w/o the lockdep warning. However, it looks
like this change will now result in inconsistent behavior compared to the
normal sized hugepages, regarding surplus page allocation. Setting nid to
numa_mem_id() means that only the node of the current CPU will be considered
for allocating a gigantic page, as opposed to just "preferring" the current
node in the normal size case (__hugetlb_alloc_buddy_huge_page() ->
alloc_pages_node()) with a fallback to using other nodes.
I am not really familiar with NUMA, and I might be wrong here, but if
this is true then gigantic pages, which may be hard allocate at runtime
in general, will be even harder to find (as surplus pages) because you
only look on the current node.
I honestly do not understand why alloc_gigantic_page() needs a nid
parameter at all, since it looks like it will only be called from
alloc_fresh_gigantic_page_node(), which in turn is only called
from alloc_fresh_gigantic_page() in a "for_each_node" loop (at least
before your patch).
Now it could be an option to also use alloc_fresh_gigantic_page()
in your patch, instead of directly calling alloc_gigantic_page(),
in __alloc_huge_page(). This would fix the "local node only" issue,
but I am not sure how to handle the required nodes_allowed parameter.
Maybe someone with more NUMA insight could have a look at this.
The patch as it is also seems to work, with the "local node only"
restriction, so it may be an option to just accept this restriction.
^ permalink raw reply
* [PATCH] fpga zynq: Check the bitstream for validity
From: Jason Gunthorpe @ 2016-11-09 15:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <609bc971-bb6d-8cd2-8e64-23c0082a6216@topic.nl>
On Wed, Nov 09, 2016 at 03:21:52PM +0100, Mike Looijmans wrote:
> I think the basic idea behind the commit is flawed to begin with and the
> patch should be discarded completely. The same discussion was done for the
> Xilinx FPGA manager driver, which resulted in the decision that the tooling
> should create a proper file.
That hasn't changed at all, this just produces a clear and obvious
message about what is wrong instead of 'timed out'.
Remember, again, the Xilinx tools do not produce an acceptable
bitstream file by default. You need to do very strange and special
things to get something acceptable to this driver. It is a very easy
mistake to make and hard to track down if you don't already know these
details.
> This driver should either become obsolete or at least move into the
> same direction as the FPGA manager rather than away from that.
I don't understand what you are talking about here, this is a fpga mgr
driver already, and is consistent with the FPGA manger - the auto
detect stuff was removed a while ago and isn't coming back.
It is perfectly reasonable for fpga manager drivers to pre-validate
the proposed bitstream, IMHO all of the drivers should try and do
that step.
Remember, it is deeply unlikely but sending garbage to an FPGA could
damage it.
> Besides political arguments, there's a more pressing technical argument
> against this theck as well: The whole check is pointless since the hardware
> itself already verifies the validity of the stream.
The purpose of the check is not to protect the hardware. The check is
to help the user provide the correct input because the hardware
provides no feedback at all on what is wrong with the input.
And again, the out-of-tree Xilinx driver accepted files that this
driver does not, so having a clear and understandable message is going
to be very important for users.
> doesn't have any effect, the hardware will discard it. There's no reason to
> waste CPU cycles duplicating this check in software.
In the typical success case we are talking about 5 32 bit compares,
which isn't even worth considering.
Jason
^ permalink raw reply
* [PATCH] fpga zynq: Check the bitstream for validity
From: Jason Gunthorpe @ 2016-11-09 16:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <ab7684fe-2407-330e-9001-21dccc781983@suse.com>
On Wed, Nov 09, 2016 at 04:18:29PM +0100, Matthias Brugger wrote:
> I get your point. Especially looping to the whole file to find the sync
> header can take a while, especially if the file is big and the sync header
> is not present.
Er, no not at all. If you send garbage to the FPGA the driver detects
it after a 2.5s timeout. The memory scan is always alot faster.
In the normal success case it is ~5 compares.
> So I think the whole idea behind this patch is to provide feedback to the
> user about what went wrong when trying to update the FPGA. Is there a way to
> get this information from the hardware which discards the update?
No, not with such specificity.
Jason
^ permalink raw reply
* [PATCH 3/3] ipmi/bt-bmc: add a sysfs file to configure a maximum response time
From: Corey Minyard @ 2016-11-09 16:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <aa4804b0-4084-6d34-c1db-68bdb5efb20a@kaod.org>
On 11/09/2016 08:42 AM, C?dric Le Goater wrote:
> On 11/07/2016 07:37 PM, Corey Minyard wrote:
>> Sorry, I was at Plumbers and extra busy with other stuff. Just getting around to reviewing this.
>>
>> On 11/02/2016 02:57 AM, C?dric Le Goater wrote:
>>> We could also use an ioctl for that purpose. sysfs seems a better
>>> approach.
>>>
>>> Signed-off-by: C?dric Le Goater <clg@kaod.org>
>>> ---
>>> drivers/char/ipmi/bt-bmc.c | 31 +++++++++++++++++++++++++++++++
>>> 1 file changed, 31 insertions(+)
>>>
>>> diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c
>>> index e751e4a754b7..d7146f0e900e 100644
>>> --- a/drivers/char/ipmi/bt-bmc.c
>>> +++ b/drivers/char/ipmi/bt-bmc.c
>>> @@ -84,6 +84,33 @@ struct bt_bmc {
>>> static atomic_t open_count = ATOMIC_INIT(0);
>>> +static ssize_t bt_bmc_show_response_time(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct bt_bmc *bt_bmc = dev_get_drvdata(dev);
>>> +
>>> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", bt_bmc->response_time);
>>> +}
>>> +
>>> +static ssize_t bt_bmc_set_response_time(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t count)
>>> +{
>>> + struct bt_bmc *bt_bmc = dev_get_drvdata(dev);
>>> + unsigned long val;
>>> + int err;
>>> +
>>> + err = kstrtoul(buf, 0, &val);
>>> + if (err)
>>> + return err;
>>> + bt_bmc->response_time = val;
>>> + return count;
>>> +}
>>> +
>>> +static DEVICE_ATTR(response_time, 0644,
>>> + bt_bmc_show_response_time, bt_bmc_set_response_time);
>>> +
>>> static u8 bt_inb(struct bt_bmc *bt_bmc, int reg)
>>> {
>>> return ioread8(bt_bmc->base + reg);
>>> @@ -572,6 +599,10 @@ static int bt_bmc_probe(struct platform_device *pdev)
>>> bt_bmc_config_irq(bt_bmc, pdev);
>>> bt_bmc->response_time = BT_BMC_RESPONSE_TIME;
>>> + rc = device_create_file(&pdev->dev, &dev_attr_response_time);
>>> + if (rc)
>>> + dev_warn(&pdev->dev, "can't create response_time file\n");
>>> +
>> You added an extra space here.
> yes. I will remove it in the next version.
>
> The patch raises a few questions on the "BT Interface Capabilities" :
>
> - should we use sysfs or a specific ioctl to configure the driver ?
I should probably say sysfs, but really I don't care. The hard part about
ioctls is the compat, and there shouldn't be any compat issues with this
interface. An ioctl is probably easier, especially if you add an ioctl for
the header size thing I talked about in the previous email.
The only thing that matters to the driver is the timeout. If you want to
make the timeout per fd, then it will have to be an ioctl.
> - should the driver handle directly the response to the "Get BT
> Interface Capabilities" command ?
That probably belongs in userspace. The only reason I can think of
to have it in the driver would be so the host driver can fetch it if the
BMC userspace is down, but I can't see how that's a good idea.
Hope my wishy-washy answer helps :-).
-corey
> What is your opinion ?
>
> Thanks,
>
> C.
>
>>> if (bt_bmc->irq) {
>>> dev_info(dev, "Using IRQ %d\n", bt_bmc->irq);
>>
^ permalink raw reply
* [PATCH 1/3] ipmi/bt-bmc: change compatible node to 'aspeed, ast2400-ibt-bmc'
From: Arnd Bergmann @ 2016-11-09 16:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <c749ff0a-bc71-bc9f-0e8c-326db2cea0fb@acm.org>
On Tuesday, November 8, 2016 12:15:57 PM CET Corey Minyard wrote:
> On 11/08/2016 09:52 AM, C?dric Le Goater wrote:
> > O
> snip
>
> >>>> While we're modifying the binding, should we add a compat string for
> >>>> the ast2500?
> >>> Well, if the change in this patch is fine for all, may be we can add
> >>> the ast2500 compat string in a followup patch ?
> >> Sounds good to me.
> > OK. So, how do we proceed with this patch ? Who would include it in its
> > tree ?
>
> I don't have anything for 4.9 at the moment. Arnd, if you have
> something, can
> you take this? Otherwise I will.
>
> And I guess I should add:
>
> Acked-by: Corey Minyard <cminyard@mvista.com>
Thanks, I've added it to my todo folder.
Olof, if you do fixes before I do, please pick up this patch with
Corey's Ack.
Arnd
^ permalink raw reply
* [PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA
From: Gabriele Paoloni @ 2016-11-09 16:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161109113955.GD10219@e106497-lin.cambridge.arm.com>
Hi Liviu
Thanks for reviewing
> -----Original Message-----
> From: liviu.dudau at arm.com [mailto:liviu.dudau at arm.com]
> Sent: 09 November 2016 11:40
> To: Yuanzhichang
> Cc: catalin.marinas at arm.com; will.deacon at arm.com; robh+dt at kernel.org;
> bhelgaas at google.com; mark.rutland at arm.com; olof at lixom.net;
> arnd at arndb.de; linux-arm-kernel at lists.infradead.org;
> lorenzo.pieralisi at arm.com; linux-kernel at vger.kernel.org; Linuxarm;
> devicetree at vger.kernel.org; linux-pci at vger.kernel.org; linux-
> serial at vger.kernel.org; minyard at acm.org; benh at kernel.crashing.org;
> zourongrong at gmail.com; John Garry; Gabriele Paoloni;
> zhichang.yuan02 at gmail.com; kantyzc at 163.com; xuwei (O)
> Subject: Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for
> special ISA
>
> On Tue, Nov 08, 2016 at 11:47:08AM +0800, zhichang.yuan wrote:
> > This patch solves two issues:
> > 1) parse and get the right I/O range from DTS node whose parent does
> not
> > define the corresponding ranges property;
> >
> > There are some special ISA/LPC devices that work on a specific I/O
> range where
> > it is not correct to specify a ranges property in DTS parent node as
> cpu
> > addresses translated from DTS node are only for memory space on some
> > architectures, such as Arm64. Without the parent 'ranges' property,
> current
> > of_translate_address() return an error.
> > Here we add a fixup function, of_get_isa_indirect_io(). During the OF
> address
> > translation, this fixup will be called to check the 'reg' address to
> be
> > translating is for those sepcial ISA/LPC devices and get the I/O
> range
> > directly from the 'reg' property.
> >
> > 2) eliminate the I/O range conflict risk with PCI/PCIE leagecy I/O
> device;
> >
> > The current __of_address_to_resource() always translates the I/O
> range to PIO.
> > But this processing is not suitable for our ISA/LPC devices whose I/O
> range is
> > not cpu address(Arnd had stressed this in his comments on V2,V3
> patch-set).
> > Here, we bypass the mapping between cpu address and PIO for the
> special
> > ISA/LPC devices. But to drive these ISA/LPC devices, a I/O port
> address below
> > PCIBIOS_MIN_IO is needed by in*/out*(). Which means there is conflict
> risk
> > between I/O range of [0, PCIBIOS_MIN_IO) and PCI/PCIE legacy I/O
> range of [0,
> > IO_SPACE_LIMIT).
> > To avoid the I/O conflict, this patch reserve the I/O range below
> > PCIBIOS_MIN_IO.
> >
> > Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
> > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> > ---
> > .../arm/hisilicon/hisilicon-low-pin-count.txt | 31 ++++++++++++
> > arch/arm64/include/asm/io.h | 6 +++
> > arch/arm64/kernel/extio.c | 25 ++++++++++
> > drivers/of/address.c | 56
> +++++++++++++++++++++-
> > drivers/pci/pci.c | 6 +--
> > include/linux/of_address.h | 17 +++++++
> > include/linux/pci.h | 8 ++++
> > 7 files changed, 145 insertions(+), 4 deletions(-)
> > create mode 100644
> Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-
> count.txt
> >
> > diff --git
> a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-
> count.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-
> low-pin-count.txt
> > new file mode 100644
> > index 0000000..13c8ddd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-
> pin-count.txt
> > @@ -0,0 +1,31 @@
> > +Hisilicon Hip06 low-pin-count device
> > + Usually LPC controller is part of PCI host bridge, so the legacy
> ISA ports
> > + locate on LPC bus can be accessed direclty. But some SoCs have
> independent
> > + LPC controller, and access the legacy ports by triggering LPC I/O
> cycles.
> > + Hisilicon Hip06 implements this LPC device.
> > +
> > +Required properties:
> > +- compatible: should be "hisilicon,low-pin-count"
> > +- #address-cells: must be 2 which stick to the ISA/EISA binding doc.
> > +- #size-cells: must be 1 which stick to the ISA/EISA binding doc.
> > +- reg: base memory range where the register set of this device is
> mapped.
> > +
> > +Note:
> > + The node name before '@' must be "isa" to represent the binding
> stick to the
> > + ISA/EISA binding specification.
> > +
> > +Example:
> > +
> > +isa at a01b0000 {
> > + compatible = "hisilicom,low-pin-count";
> > + #address-cells = <2>;
> > + #size-cells = <1>;
> > + reg = <0x0 0xa01b0000 0x0 0x1000>;
> > +
> > + ipmi0: bt at e4 {
> > + compatible = "ipmi-bt";
> > + device_type = "ipmi";
> > + reg = <0x01 0xe4 0x04>;
> > + status = "disabled";
> > + };
> > +};
>
>
> This documentation file needs to be part of the next patch. It has
> nothing to do with
> what you are trying to fix here.
Yes you're right...we'll move it to next one
>
>
> > diff --git a/arch/arm64/include/asm/io.h
> b/arch/arm64/include/asm/io.h
> > index 136735d..c26b7cc 100644
> > --- a/arch/arm64/include/asm/io.h
> > +++ b/arch/arm64/include/asm/io.h
> > @@ -175,6 +175,12 @@ static inline u64 __raw_readq(const volatile
> void __iomem *addr)
> > #define outsl outsl
> >
> > DECLARE_EXTIO(l, u32)
> > +
> > +#define indirect_io_enabled indirect_io_enabled
> > +extern bool indirect_io_enabled(void);
> > +
> > +#define addr_is_indirect_io addr_is_indirect_io
> > +extern int addr_is_indirect_io(u64 taddr);
> > #endif
> >
> >
> > diff --git a/arch/arm64/kernel/extio.c b/arch/arm64/kernel/extio.c
> > index 647b3fa..3d45fa8 100644
> > --- a/arch/arm64/kernel/extio.c
> > +++ b/arch/arm64/kernel/extio.c
> > @@ -19,6 +19,31 @@
> >
> > struct extio_ops *arm64_extio_ops;
> >
> > +/**
> > + * indirect_io_enabled - check whether indirectIO is enabled.
> > + * arm64_extio_ops will be set only when indirectIO mechanism had
> been
> > + * initialized.
> > + *
> > + * Returns true when indirectIO is enabled.
> > + */
> > +bool indirect_io_enabled(void)
> > +{
> > + return arm64_extio_ops ? true : false;
> > +}
> > +
> > +/**
> > + * addr_is_indirect_io - check whether the input taddr is for
> indirectIO.
> > + * @taddr: the io address to be checked.
> > + *
> > + * Returns 1 when taddr is in the range; otherwise return 0.
> > + */
> > +int addr_is_indirect_io(u64 taddr)
> > +{
> > + if (arm64_extio_ops->start > taddr || arm64_extio_ops->end <
> taddr)
>
> start >= taddr ?
Nope... if (taddr < arm64_extio_ops->start || taddr > arm64_extio_ops->end)
then taddr is outside the range [start; end] and will return 0; otherwise
it will return 1...
>
> > + return 0;
> > +
> > + return 1;
> > +}
> >
> > BUILD_EXTIO(b, u8)
> >
> > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > index 02b2903..cc2a05d 100644
> > --- a/drivers/of/address.c
> > +++ b/drivers/of/address.c
> > @@ -479,6 +479,50 @@ static int of_empty_ranges_quirk(struct
> device_node *np)
> > return false;
> > }
> >
> > +
> > +/*
> > + * of_isa_indirect_io - get the IO address from some isa reg
> property value.
> > + * For some isa/lpc devices, no ranges property in ancestor node.
> > + * The device addresses are described directly in their regs
> property.
> > + * This fixup function will be called to get the IO address of
> isa/lpc
> > + * devices when the normal of_translation failed.
> > + *
> > + * @parent: points to the parent dts node;
> > + * @bus: points to the of_bus which can be used to parse
> address;
> > + * @addr: the address from reg property;
> > + * @na: the address cell counter of @addr;
> > + * @presult: store the address paresed from @addr;
> > + *
> > + * return 1 when successfully get the I/O address;
> > + * 0 will return for some failures.
>
> Bah, you are returning a signed int, why 0 for failure? Return a
> negative value with
> error codes. Otherwise change the return value into a bool.
Yes we'll move to bool
>
> > + */
> > +static int of_get_isa_indirect_io(struct device_node *parent,
> > + struct of_bus *bus, __be32 *addr,
> > + int na, u64 *presult)
> > +{
> > + unsigned int flags;
> > + unsigned int rlen;
> > +
> > + /* whether support indirectIO */
> > + if (!indirect_io_enabled())
> > + return 0;
> > +
> > + if (!of_bus_isa_match(parent))
> > + return 0;
> > +
> > + flags = bus->get_flags(addr);
> > + if (!(flags & IORESOURCE_IO))
> > + return 0;
> > +
> > + /* there is ranges property, apply the normal translation
> directly. */
>
> s/there is ranges/if we have a 'ranges'/
Thanks for spotting this
>
> > + if (of_get_property(parent, "ranges", &rlen))
> > + return 0;
> > +
> > + *presult = of_read_number(addr + 1, na - 1);
> > + /* this fixup is only valid for specific I/O range. */
> > + return addr_is_indirect_io(*presult);
> > +}
> > +
> > static int of_translate_one(struct device_node *parent, struct
> of_bus *bus,
> > struct of_bus *pbus, __be32 *addr,
> > int na, int ns, int pna, const char *rprop)
> > @@ -595,6 +639,15 @@ static u64 __of_translate_address(struct
> device_node *dev,
> > result = of_read_number(addr, na);
> > break;
> > }
> > + /*
> > + * For indirectIO device which has no ranges property, get
> > + * the address from reg directly.
> > + */
> > + if (of_get_isa_indirect_io(dev, bus, addr, na, &result)) {
> > + pr_debug("isa indirectIO matched(%s)..addr =
> 0x%llx\n",
> > + of_node_full_name(dev), result);
> > + break;
> > + }
> >
> > /* Get new parent bus and counts */
> > pbus = of_match_bus(parent);
> > @@ -688,8 +741,9 @@ static int __of_address_to_resource(struct
> device_node *dev,
> > if (taddr == OF_BAD_ADDR)
> > return -EINVAL;
> > memset(r, 0, sizeof(struct resource));
> > - if (flags & IORESOURCE_IO) {
> > + if (flags & IORESOURCE_IO && taddr >= PCIBIOS_MIN_IO) {
> > unsigned long port;
> > +
> > port = pci_address_to_pio(taddr);
> > if (port == (unsigned long)-1)
> > return -EINVAL;
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index ba34907..1a08511 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -3263,7 +3263,7 @@ int __weak pci_register_io_range(phys_addr_t
> addr, resource_size_t size)
> >
> > #ifdef PCI_IOBASE
> > struct io_range *range;
> > - resource_size_t allocated_size = 0;
> > + resource_size_t allocated_size = PCIBIOS_MIN_IO;
> >
> > /* check if the range hasn't been previously recorded */
> > spin_lock(&io_range_lock);
> > @@ -3312,7 +3312,7 @@ phys_addr_t pci_pio_to_address(unsigned long
> pio)
> >
> > #ifdef PCI_IOBASE
> > struct io_range *range;
> > - resource_size_t allocated_size = 0;
> > + resource_size_t allocated_size = PCIBIOS_MIN_IO;
>
> Have you checked that pci_pio_to_address still returns valid values
> after this? I know that
> you are trying to take into account PCIBIOS_MIN_IO limit when
> allocating reserving the IO ranges,
> but the values added in the io_range_list are still starting from zero,
> no from PCIBIOS_MIN_IO,
I think you're wrong here as in pci_address_to_pio we have:
+ resource_size_t offset = PCIBIOS_MIN_IO;
This should be enough to guarantee that the PIOs start at
PCIBIOS_MIN_IO...right?
> so the calculation of the address in this function could return
> negative values casted to pci_addr_t.
>
> Maybe you want to adjust the range->start value in
> pci_register_io_range() as well to have it
> offset by PCIBIOS_MIN_IO as well.
>
> Best regards,
> Liviu
>
> >
> > if (pio > IO_SPACE_LIMIT)
> > return address;
> > @@ -3335,7 +3335,7 @@ unsigned long __weak
> pci_address_to_pio(phys_addr_t address)
> > {
> > #ifdef PCI_IOBASE
> > struct io_range *res;
> > - resource_size_t offset = 0;
> > + resource_size_t offset = PCIBIOS_MIN_IO;
> > unsigned long addr = -1;
> >
> > spin_lock(&io_range_lock);
> > diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> > index 3786473..deec469 100644
> > --- a/include/linux/of_address.h
> > +++ b/include/linux/of_address.h
> > @@ -24,6 +24,23 @@ struct of_pci_range {
> > #define for_each_of_pci_range(parser, range) \
> > for (; of_pci_range_parser_one(parser, range);)
> >
> > +
> > +#ifndef indirect_io_enabled
> > +#define indirect_io_enabled indirect_io_enabled
> > +static inline bool indirect_io_enabled(void)
> > +{
> > + return false;
> > +}
> > +#endif
> > +
> > +#ifndef addr_is_indirect_io
> > +#define addr_is_indirect_io addr_is_indirect_io
> > +static inline int addr_is_indirect_io(u64 taddr)
> > +{
> > + return 0;
> > +}
> > +#endif
> > +
> > /* Translate a DMA address from device space to CPU space */
> > extern u64 of_translate_dma_address(struct device_node *dev,
> > const __be32 *in_addr);
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 0e49f70..7f6bbb6 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -2130,4 +2130,12 @@ static inline bool pci_ari_enabled(struct
> pci_bus *bus)
> > /* provide the legacy pci_dma_* API */
> > #include <linux/pci-dma-compat.h>
> >
> > +/*
> > + * define this macro here to refrain from compilation error for some
> > + * platforms. Please keep this macro at the end of this header file.
> > + */
> > +#ifndef PCIBIOS_MIN_IO
> > +#define PCIBIOS_MIN_IO 0
> > +#endif
> > +
> > #endif /* LINUX_PCI_H */
> > --
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci"
> in
> > the body of a message to majordomo at vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> ====================
> | I would like to |
> | fix the world, |
> | but they're not |
> | giving me the |
> \ source code! /
> ---------------
> ?\_(?)_/?
^ permalink raw reply
* [PATCH] ASoC: sun4i-codec: fix semicolon.cocci warnings
From: kbuild test robot @ 2016-11-09 16:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <201611100003.EAHNoIbz%fengguang.wu@intel.com>
sound/soc/sunxi/sun4i-codec.c:1339:2-3: Unneeded semicolon
Remove unneeded semicolon.
Generated by: scripts/coccinelle/misc/semicolon.cocci
CC: Chen-Yu Tsai <wens@csie.org>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
sun4i-codec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/sound/soc/sunxi/sun4i-codec.c
+++ b/sound/soc/sunxi/sun4i-codec.c
@@ -1336,7 +1336,7 @@ static int sun4i_codec_probe(struct plat
dev_err(&pdev->dev, "Failed to get reset control\n");
return PTR_ERR(scodec->rst);
}
- };
+ }
scodec->gpio_pa = devm_gpiod_get_optional(&pdev->dev, "allwinner,pa",
GPIOD_OUT_LOW);
^ permalink raw reply
* [PATCH] Replacement for Arm initrd memblock reserve and free inconsistency.
From: william.helsby at stfc.ac.uk @ 2016-11-09 16:35 UTC (permalink / raw)
To: linux-arm-kernel
A boot time system crash was noticed with a segmentation fault just after the initrd image had been used to initialise the ramdisk.
This occurred when the U-Boot loaded the ramdisk image from a FAT partition, but not when loaded by TFTPBOOT. This is not understood?
However the problem was caused by free_initrd_mem freeing and "poisoning" memory that had been allocted to init/main.c to store the saved_command_line
This patch reverses "ARM: 8167/1: extend the reserved memory for initrd to be page aligned" because it is safer to leave a partial head or tail page reserved (wasted) than to free a page which is partially still in use.
If this is not acceptable (particularly if wanting large contiguous physical areas for DMA) then a better solution is required.
This would extend the region reserved to page boundaries, if possible without overlapping other regions. My previous attempt to fix this coded this scheme, to grow the are reserved.
However, this? again is not safe if in growing the area it then overlaps a region that is in use.
Note this path is against the 4.6 kernel, but as far as I can tell applies equally to 4.8.
Signed-off-by: William Helsby <wih73@xilinxsrv1.dl.ac.uk>
---
arch/arm/mm/init.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 370581a..ff3e9c3 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -770,12 +770,6 @@ static int keep_initrd;
void free_initrd_mem(unsigned long start, unsigned long end)
{
??????? if (!keep_initrd) {
-?????????????? if (start == initrd_start)
-?????????????????????? start = round_down(start, PAGE_SIZE);
-?????????????? if (end == initrd_end)
-?????????????????????? end = round_up(end, PAGE_SIZE);
-
-???? ??????????poison_init_mem((void *)start, PAGE_ALIGN(end) - start);
??????????????? free_reserved_area((void *)start, (void *)end, -1, "initrd");
??????? }
}
--
1.8.3.1
Science and Technology Facilities Council
SciTech Daresbury
Keckwick Lane
Daresbury
Warrington WA4 4AD
Tel +44(0)1925 603250
^ permalink raw reply related
* [PATCH] i.MX: Kconfig: Drop errata 769419 for Vybrid
From: Andrey Smirnov @ 2016-11-09 16:44 UTC (permalink / raw)
To: linux-arm-kernel
According to the datasheet, VF610 uses revision r3p2 of the L2C-310
block, same as i.MX6Q+, which does not require a software workaround for
ARM errata 769419.
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
arch/arm/mach-imx/Kconfig | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index 9155b63..936c59d 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -557,7 +557,6 @@ config SOC_VF610
bool "Vybrid Family VF610 support"
select ARM_GIC if ARCH_MULTI_V7
select PINCTRL_VF610
- select PL310_ERRATA_769419 if CACHE_L2X0
help
This enables support for Freescale Vybrid VF610 processor.
--
2.5.5
^ permalink raw reply related
* [PATCH] ata: xgene: Enable NCQ support for APM X-Gene SATA controller hardware v1.1
From: Tejun Heo @ 2016-11-09 16:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAFd313x-mXpy0N3aKx2rfoag1FNOrN-LzKb5UHkY2L+GGc0B-A@mail.gmail.com>
Hello,
On Wed, Sep 14, 2016 at 04:15:00PM +0530, Rameshwar Sahu wrote:
> > @@ -821,8 +823,6 @@ static int xgene_ahci_probe(struct platform_device
> > *pdev)
> > dev_warn(&pdev->dev, "%s: Error reading
> > device info. Assume version1\n",
> > __func__);
> > version = XGENE_AHCI_V1;
> > - } else if (info->valid & ACPI_VALID_CID) {
> > - version = XGENE_AHCI_V2;
Can you please explain this part a bit? Everything else looks good to
me.
Thanks.
--
tejun
^ permalink raw reply
* [PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA
From: liviu.dudau at arm.com @ 2016-11-09 16:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <EE11001F9E5DDD47B7634E2F8A612F2E1F8F27C0@lhreml507-mbx>
On Wed, Nov 09, 2016 at 04:16:17PM +0000, Gabriele Paoloni wrote:
> Hi Liviu
>
> Thanks for reviewing
>
[removed some irrelevant part of discussion, avoid crazy formatting]
> > > +/**
> > > + * addr_is_indirect_io - check whether the input taddr is for
> > indirectIO.
> > > + * @taddr: the io address to be checked.
> > > + *
> > > + * Returns 1 when taddr is in the range; otherwise return 0.
> > > + */
> > > +int addr_is_indirect_io(u64 taddr)
> > > +{
> > > + if (arm64_extio_ops->start > taddr || arm64_extio_ops->end <
> > taddr)
> >
> > start >= taddr ?
>
> Nope... if (taddr < arm64_extio_ops->start || taddr > arm64_extio_ops->end)
> then taddr is outside the range [start; end] and will return 0; otherwise
> it will return 1...
Oops, sorry, did not pay attention to the returned value. The check is
correct as it is, no need to change then.
>
> >
> > > + return 0;
> > > +
> > > + return 1;
> > > +}
> > >
> > > BUILD_EXTIO(b, u8)
> > >
> > > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > > index 02b2903..cc2a05d 100644
> > > --- a/drivers/of/address.c
> > > +++ b/drivers/of/address.c
> > > @@ -479,6 +479,50 @@ static int of_empty_ranges_quirk(struct
> > device_node *np)
> > > return false;
> > > }
> > >
> > > +
> > > +/*
> > > + * of_isa_indirect_io - get the IO address from some isa reg
> > property value.
> > > + * For some isa/lpc devices, no ranges property in ancestor node.
> > > + * The device addresses are described directly in their regs
> > property.
> > > + * This fixup function will be called to get the IO address of
> > isa/lpc
> > > + * devices when the normal of_translation failed.
> > > + *
> > > + * @parent: points to the parent dts node;
> > > + * @bus: points to the of_bus which can be used to parse
> > address;
> > > + * @addr: the address from reg property;
> > > + * @na: the address cell counter of @addr;
> > > + * @presult: store the address paresed from @addr;
> > > + *
> > > + * return 1 when successfully get the I/O address;
> > > + * 0 will return for some failures.
> >
> > Bah, you are returning a signed int, why 0 for failure? Return a
> > negative value with
> > error codes. Otherwise change the return value into a bool.
>
> Yes we'll move to bool
>
> >
> > > + */
> > > +static int of_get_isa_indirect_io(struct device_node *parent,
> > > + struct of_bus *bus, __be32 *addr,
> > > + int na, u64 *presult)
> > > +{
> > > + unsigned int flags;
> > > + unsigned int rlen;
> > > +
> > > + /* whether support indirectIO */
> > > + if (!indirect_io_enabled())
> > > + return 0;
> > > +
> > > + if (!of_bus_isa_match(parent))
> > > + return 0;
> > > +
> > > + flags = bus->get_flags(addr);
> > > + if (!(flags & IORESOURCE_IO))
> > > + return 0;
> > > +
> > > + /* there is ranges property, apply the normal translation
> > directly. */
> >
> > s/there is ranges/if we have a 'ranges'/
>
> Thanks for spotting this
>
> >
> > > + if (of_get_property(parent, "ranges", &rlen))
> > > + return 0;
> > > +
> > > + *presult = of_read_number(addr + 1, na - 1);
> > > + /* this fixup is only valid for specific I/O range. */
> > > + return addr_is_indirect_io(*presult);
> > > +}
> > > +
> > > static int of_translate_one(struct device_node *parent, struct
> > of_bus *bus,
> > > struct of_bus *pbus, __be32 *addr,
> > > int na, int ns, int pna, const char *rprop)
> > > @@ -595,6 +639,15 @@ static u64 __of_translate_address(struct
> > device_node *dev,
> > > result = of_read_number(addr, na);
> > > break;
> > > }
> > > + /*
> > > + * For indirectIO device which has no ranges property, get
> > > + * the address from reg directly.
> > > + */
> > > + if (of_get_isa_indirect_io(dev, bus, addr, na, &result)) {
> > > + pr_debug("isa indirectIO matched(%s)..addr =
> > 0x%llx\n",
> > > + of_node_full_name(dev), result);
> > > + break;
> > > + }
> > >
> > > /* Get new parent bus and counts */
> > > pbus = of_match_bus(parent);
> > > @@ -688,8 +741,9 @@ static int __of_address_to_resource(struct
> > device_node *dev,
> > > if (taddr == OF_BAD_ADDR)
> > > return -EINVAL;
> > > memset(r, 0, sizeof(struct resource));
> > > - if (flags & IORESOURCE_IO) {
> > > + if (flags & IORESOURCE_IO && taddr >= PCIBIOS_MIN_IO) {
> > > unsigned long port;
> > > +
> > > port = pci_address_to_pio(taddr);
> > > if (port == (unsigned long)-1)
> > > return -EINVAL;
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index ba34907..1a08511 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -3263,7 +3263,7 @@ int __weak pci_register_io_range(phys_addr_t
> > addr, resource_size_t size)
> > >
> > > #ifdef PCI_IOBASE
> > > struct io_range *range;
> > > - resource_size_t allocated_size = 0;
> > > + resource_size_t allocated_size = PCIBIOS_MIN_IO;
> > >
> > > /* check if the range hasn't been previously recorded */
> > > spin_lock(&io_range_lock);
> > > @@ -3312,7 +3312,7 @@ phys_addr_t pci_pio_to_address(unsigned long
> > pio)
> > >
> > > #ifdef PCI_IOBASE
> > > struct io_range *range;
> > > - resource_size_t allocated_size = 0;
> > > + resource_size_t allocated_size = PCIBIOS_MIN_IO;
> >
> > Have you checked that pci_pio_to_address still returns valid values
> > after this? I know that
> > you are trying to take into account PCIBIOS_MIN_IO limit when
> > allocating reserving the IO ranges,
> > but the values added in the io_range_list are still starting from zero,
> > no from PCIBIOS_MIN_IO,
>
> I think you're wrong here as in pci_address_to_pio we have:
> + resource_size_t offset = PCIBIOS_MIN_IO;
>
> This should be enough to guarantee that the PIOs start at
> PCIBIOS_MIN_IO...right?
I don't think you can guarantee that the pio value that gets passed into
pci_pio_to_address() always comes from a previously returned value by
pci_address_to_pio(). Maybe you can add a check in pci_pio_to_address()
if (pio < PCIBIOS_MIN_IO)
return address;
to avoid adding more checks in the list_for_each_entry() loop.
Best regards,
Liviu
>
>
> > so the calculation of the address in this function could return
> > negative values casted to pci_addr_t.
> >
> > Maybe you want to adjust the range->start value in
> > pci_register_io_range() as well to have it
> > offset by PCIBIOS_MIN_IO as well.
> >
> > Best regards,
> > Liviu
> >
> > >
> > > if (pio > IO_SPACE_LIMIT)
> > > return address;
> > > @@ -3335,7 +3335,7 @@ unsigned long __weak
> > pci_address_to_pio(phys_addr_t address)
> > > {
> > > #ifdef PCI_IOBASE
> > > struct io_range *res;
> > > - resource_size_t offset = 0;
> > > + resource_size_t offset = PCIBIOS_MIN_IO;
> > > unsigned long addr = -1;
> > >
> > > spin_lock(&io_range_lock);
> > > diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> > > index 3786473..deec469 100644
> > > --- a/include/linux/of_address.h
> > > +++ b/include/linux/of_address.h
> > > @@ -24,6 +24,23 @@ struct of_pci_range {
> > > #define for_each_of_pci_range(parser, range) \
> > > for (; of_pci_range_parser_one(parser, range);)
> > >
> > > +
> > > +#ifndef indirect_io_enabled
> > > +#define indirect_io_enabled indirect_io_enabled
> > > +static inline bool indirect_io_enabled(void)
> > > +{
> > > + return false;
> > > +}
> > > +#endif
> > > +
> > > +#ifndef addr_is_indirect_io
> > > +#define addr_is_indirect_io addr_is_indirect_io
> > > +static inline int addr_is_indirect_io(u64 taddr)
> > > +{
> > > + return 0;
> > > +}
> > > +#endif
> > > +
> > > /* Translate a DMA address from device space to CPU space */
> > > extern u64 of_translate_dma_address(struct device_node *dev,
> > > const __be32 *in_addr);
> > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > index 0e49f70..7f6bbb6 100644
> > > --- a/include/linux/pci.h
> > > +++ b/include/linux/pci.h
> > > @@ -2130,4 +2130,12 @@ static inline bool pci_ari_enabled(struct
> > pci_bus *bus)
> > > /* provide the legacy pci_dma_* API */
> > > #include <linux/pci-dma-compat.h>
> > >
> > > +/*
> > > + * define this macro here to refrain from compilation error for some
> > > + * platforms. Please keep this macro at the end of this header file.
> > > + */
> > > +#ifndef PCIBIOS_MIN_IO
> > > +#define PCIBIOS_MIN_IO 0
> > > +#endif
> > > +
> > > #endif /* LINUX_PCI_H */
> > > --
> > > 1.9.1
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-pci"
> > in
> > > the body of a message to majordomo at vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
?\_(?)_/?
^ permalink raw reply
* [PATCH] mfd: qcom-pm8xxx: Clean up PM8XXX namespace
From: Bjorn Andersson @ 2016-11-09 16:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477487453-15801-1-git-send-email-linus.walleij@linaro.org>
On Wed 26 Oct 06:10 PDT 2016, Linus Walleij wrote:
> The Kconfig and file naming for the PM8xxx driver is totally
> confusing:
>
> - Kconfig options MFD_PM8XXX and MFD_PM8921_CORE, some in-kernel
> users depending on or selecting either at random.
> - A driver file named pm8921-core.c even if it is indeed
> used by the whole PM8xxx family of chips.
> - An irqchip named pm8xxx since it was (I guess) realized that
> the driver was generic for all pm8xxx PMICs.
>
> As I may want to add support for PM8901 this is starting to get
> really messy. Fix this situation by:
>
> - Remove the MFD_PM8921_CORE symbol and rely solely on MFD_PM8XXX
> and convert all users, including LEDs Kconfig and ARM defconfigs
> for qcom and multi_v7 to use that single symbol.
> - Renaming the driver to qcom-pm8xxx.c to fit along the two
> other qcom* prefixed drivers.
> - Rename functions withing the driver from 8921 to 8xxx to
> indicate it is generic.
> - Just drop the =m config from the pxa_defconfig, I have no clue
> why it is even there, it is not a Qualcomm platform. (Possibly
> older Kconfig noise from saveconfig.)
>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Regards,
Bjorn
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> I do NOT think it is a good idea to try to split this commit up,
> I rather prefer that Lee simply merge it into MFD.
>
> The reason is that files like qcom_defconfig already contain both
> the right symbols, but the MFD_PM8921_CORE symbol cannot be removed
> until this rename has happened, whereas multi_v7_defconfig needs
> it added etc, and this is just a clean nice cut.
>
> Jacek, ARM SoC person: please ACK this patch to get merged into
> MFD.
> ---
> arch/arm/configs/multi_v7_defconfig | 2 +-
> arch/arm/configs/pxa_defconfig | 1 -
> arch/arm/configs/qcom_defconfig | 1 -
> drivers/leds/Kconfig | 2 +-
> drivers/mfd/Kconfig | 14 ++++------
> drivers/mfd/Makefile | 2 +-
> drivers/mfd/{pm8921-core.c => qcom-pm8xxx.c} | 42 ++++++++++++++--------------
> 7 files changed, 29 insertions(+), 35 deletions(-)
> rename drivers/mfd/{pm8921-core.c => qcom-pm8xxx.c} (92%)
>
> diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
> index 437d0740dec6..4280de7b9b4e 100644
> --- a/arch/arm/configs/multi_v7_defconfig
> +++ b/arch/arm/configs/multi_v7_defconfig
> @@ -489,7 +489,7 @@ CONFIG_MFD_MAX8907=y
> CONFIG_MFD_MAX8997=y
> CONFIG_MFD_MAX8998=y
> CONFIG_MFD_RK808=y
> -CONFIG_MFD_PM8921_CORE=y
> +CONFIG_MFD_PM8XXX=y
> CONFIG_MFD_QCOM_RPM=y
> CONFIG_MFD_SPMI_PMIC=y
> CONFIG_MFD_SEC_CORE=y
> diff --git a/arch/arm/configs/pxa_defconfig b/arch/arm/configs/pxa_defconfig
> index a016ecc0084b..e4314b1227a3 100644
> --- a/arch/arm/configs/pxa_defconfig
> +++ b/arch/arm/configs/pxa_defconfig
> @@ -411,7 +411,6 @@ CONFIG_MFD_MAX77693=y
> CONFIG_MFD_MAX8907=m
> CONFIG_EZX_PCAP=y
> CONFIG_UCB1400_CORE=m
> -CONFIG_MFD_PM8921_CORE=m
> CONFIG_MFD_SEC_CORE=y
> CONFIG_MFD_PALMAS=y
> CONFIG_MFD_TPS65090=y
> diff --git a/arch/arm/configs/qcom_defconfig b/arch/arm/configs/qcom_defconfig
> index c2dff4fd5fc4..74e9cd759b99 100644
> --- a/arch/arm/configs/qcom_defconfig
> +++ b/arch/arm/configs/qcom_defconfig
> @@ -119,7 +119,6 @@ CONFIG_POWER_RESET=y
> CONFIG_POWER_RESET_MSM=y
> CONFIG_THERMAL=y
> CONFIG_MFD_PM8XXX=y
> -CONFIG_MFD_PM8921_CORE=y
> CONFIG_MFD_QCOM_RPM=y
> CONFIG_MFD_SPMI_PMIC=y
> CONFIG_REGULATOR=y
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 7a628c6516f6..86bb1515a00e 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -645,7 +645,7 @@ config LEDS_VERSATILE
>
> config LEDS_PM8058
> tristate "LED Support for the Qualcomm PM8058 PMIC"
> - depends on MFD_PM8921_CORE
> + depends on MFD_PM8XXX
> depends on LEDS_CLASS
> help
> Choose this option if you want to use the LED drivers in
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index c6df6442ba2b..1ed0584f494e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -756,24 +756,20 @@ config UCB1400_CORE
> module will be called ucb1400_core.
>
> config MFD_PM8XXX
> - tristate
> -
> -config MFD_PM8921_CORE
> - tristate "Qualcomm PM8921 PMIC chip"
> + tristate "Qualcomm PM8xxx PMIC chips driver"
> depends on (ARM || HEXAGON)
> select IRQ_DOMAIN
> select MFD_CORE
> - select MFD_PM8XXX
> select REGMAP
> help
> If you say yes to this option, support will be included for the
> - built-in PM8921 PMIC chip.
> + built-in PM8xxx PMIC chips.
>
> - This is required if your board has a PM8921 and uses its features,
> + This is required if your board has a PM8xxx and uses its features,
> such as: MPPs, GPIOs, regulators, interrupts, and PWM.
>
> - Say M here if you want to include support for PM8921 chip as a module.
> - This will build a module called "pm8921-core".
> + Say M here if you want to include support for PM8xxx chips as a
> + module. This will build a module called "pm8xxx-core".
>
> config MFD_QCOM_RPM
> tristate "Qualcomm Resource Power Manager (RPM)"
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 9834e669d985..7bb5a50127cb 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -172,7 +172,7 @@ obj-$(CONFIG_MFD_SI476X_CORE) += si476x-core.o
>
> obj-$(CONFIG_MFD_CS5535) += cs5535-mfd.o
> obj-$(CONFIG_MFD_OMAP_USB_HOST) += omap-usb-host.o omap-usb-tll.o
> -obj-$(CONFIG_MFD_PM8921_CORE) += pm8921-core.o ssbi.o
> +obj-$(CONFIG_MFD_PM8XXX) += qcom-pm8xxx.o ssbi.o
> obj-$(CONFIG_MFD_QCOM_RPM) += qcom_rpm.o
> obj-$(CONFIG_MFD_SPMI_PMIC) += qcom-spmi-pmic.o
> obj-$(CONFIG_TPS65911_COMPARATOR) += tps65911-comparator.o
> diff --git a/drivers/mfd/pm8921-core.c b/drivers/mfd/qcom-pm8xxx.c
> similarity index 92%
> rename from drivers/mfd/pm8921-core.c
> rename to drivers/mfd/qcom-pm8xxx.c
> index 0e3a2ea25942..7f9620ec61e8 100644
> --- a/drivers/mfd/pm8921-core.c
> +++ b/drivers/mfd/qcom-pm8xxx.c
> @@ -53,7 +53,7 @@
> #define REG_HWREV 0x002 /* PMIC4 revision */
> #define REG_HWREV_2 0x0E8 /* PMIC4 revision 2 */
>
> -#define PM8921_NR_IRQS 256
> +#define PM8XXX_NR_IRQS 256
>
> struct pm_irq_chip {
> struct regmap *regmap;
> @@ -308,22 +308,22 @@ static const struct regmap_config ssbi_regmap_config = {
> .reg_write = ssbi_reg_write
> };
>
> -static const struct of_device_id pm8921_id_table[] = {
> +static const struct of_device_id pm8xxx_id_table[] = {
> { .compatible = "qcom,pm8018", },
> { .compatible = "qcom,pm8058", },
> { .compatible = "qcom,pm8921", },
> { }
> };
> -MODULE_DEVICE_TABLE(of, pm8921_id_table);
> +MODULE_DEVICE_TABLE(of, pm8xxx_id_table);
>
> -static int pm8921_probe(struct platform_device *pdev)
> +static int pm8xxx_probe(struct platform_device *pdev)
> {
> struct regmap *regmap;
> int irq, rc;
> unsigned int val;
> u32 rev;
> struct pm_irq_chip *chip;
> - unsigned int nirqs = PM8921_NR_IRQS;
> + unsigned int nirqs = PM8XXX_NR_IRQS;
>
> irq = platform_get_irq(pdev, 0);
> if (irq < 0)
> @@ -384,46 +384,46 @@ static int pm8921_probe(struct platform_device *pdev)
> return rc;
> }
>
> -static int pm8921_remove_child(struct device *dev, void *unused)
> +static int pm8xxx_remove_child(struct device *dev, void *unused)
> {
> platform_device_unregister(to_platform_device(dev));
> return 0;
> }
>
> -static int pm8921_remove(struct platform_device *pdev)
> +static int pm8xxx_remove(struct platform_device *pdev)
> {
> int irq = platform_get_irq(pdev, 0);
> struct pm_irq_chip *chip = platform_get_drvdata(pdev);
>
> - device_for_each_child(&pdev->dev, NULL, pm8921_remove_child);
> + device_for_each_child(&pdev->dev, NULL, pm8xxx_remove_child);
> irq_set_chained_handler_and_data(irq, NULL, NULL);
> irq_domain_remove(chip->irqdomain);
>
> return 0;
> }
>
> -static struct platform_driver pm8921_driver = {
> - .probe = pm8921_probe,
> - .remove = pm8921_remove,
> +static struct platform_driver pm8xxx_driver = {
> + .probe = pm8xxx_probe,
> + .remove = pm8xxx_remove,
> .driver = {
> - .name = "pm8921-core",
> - .of_match_table = pm8921_id_table,
> + .name = "pm8xxx-core",
> + .of_match_table = pm8xxx_id_table,
> },
> };
>
> -static int __init pm8921_init(void)
> +static int __init pm8xxx_init(void)
> {
> - return platform_driver_register(&pm8921_driver);
> + return platform_driver_register(&pm8xxx_driver);
> }
> -subsys_initcall(pm8921_init);
> +subsys_initcall(pm8xxx_init);
>
> -static void __exit pm8921_exit(void)
> +static void __exit pm8xxx_exit(void)
> {
> - platform_driver_unregister(&pm8921_driver);
> + platform_driver_unregister(&pm8xxx_driver);
> }
> -module_exit(pm8921_exit);
> +module_exit(pm8xxx_exit);
>
> MODULE_LICENSE("GPL v2");
> -MODULE_DESCRIPTION("PMIC 8921 core driver");
> +MODULE_DESCRIPTION("PMIC 8xxx core driver");
> MODULE_VERSION("1.0");
> -MODULE_ALIAS("platform:pm8921-core");
> +MODULE_ALIAS("platform:pm8xxx-core");
> --
> 2.7.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* KASAN & the vmalloc area
From: Andrey Ryabinin @ 2016-11-09 16:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161108190302.GH15297@leverpostej>
On 11/08/2016 10:03 PM, Mark Rutland wrote:
> Hi,
>
> I see a while back [1] there was a discussion of what to do about KASAN
> and vmapped stacks, but it doesn't look like that was solved, judging by
> the vmapped stacks pull [2] for v4.9.
>
> I wondered whether anyone had looked at that since?
>
Sadly, but I didn't have much time for this yet, so it's in an initial state.
> I have an additional reason to want to dynamically allocate the vmalloc
> area shadow: it turns out that KASAN currently interacts rather poorly
> with the arm64 ptdump code.
>
> When KASAN is selected, we allocate shadow for the whole vmalloc area,
> using common zero pte, pmd, pud tables. Walking over these in the ptdump
> code takes a *very* long time (I've seen up to 15 minutes with
> KASAN_OUTLINE enabled). For DEBUG_WX [3], this means boot hangs for that
> long, too.
>
I didn't look at any code, but we probably could can remember last visited pgd
and skip next pgd if it's the same as previous.
Alternatively - just skip kasan_zero_p*d in ptdump walker.
> If I don't allocate vmalloc shadow (and remove the apparently pointlesss
> shadow of the shadow area), and only allocate shadow for the image,
> fixmap, vmemmap and so on, that delay gets cut to a few seconds, which
> is tolerable for a debug configuration...
>
> ... however, things blow up when the kernel touches vmalloc'd memory for
> the first time, as we don't install shadow for that dynamically.
>
> Thanks,
> Mark.
>
> [1] https://lkml.kernel.org/r/CALCETrWucrYp+yq8RHSDqf93xtg793duByirurzJbLRhrz=tcA at mail.gmail.com
> [2] https://lkml.kernel.org/r/20161003092940.GA691 at gmail.com
> [3] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-October/464191.html
>
^ permalink raw reply
* [PATCH v2 7/7] soc: renesas: Identify SoC and register with the SoC bus
From: Arnd Bergmann @ 2016-11-09 16:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477913455-5314-8-git-send-email-geert+renesas@glider.be>
On Monday, October 31, 2016 12:30:55 PM CET Geert Uytterhoeven wrote:
> v2:
> - Drop SoC families and family names; use fixed "Renesas" instead,
I think I'd rather have seen the family names left in there, but it's
not important, so up to you.
> - Use "renesas,prr" and "renesas,cccr" device nodes in DT if
> available, else fall back to hardcoded addresses for compatibility
> with existing DTBs,
I only see patches 2, 3, 5, and 7 in my inbox, so I'm lacking the DT
binding for these, among other things.
It does seem wrong to have a device node for a specific register though.
Shouldn't the node be for the block of registers that these are inside
of?
> - Don't register the SoC bus if the chip ID register is missing,
Why? My objection was to hardcoding the register, not to registering
the device? I think I'd rather see the device registered with an
empty revision string.
> +#define CCCR 0xe600101c /* Common Chip Code Register */
> +#define PRR 0xff000044 /* Product Register */
> +#define PRR3 0xfff00044 /* Product Register on R-Car Gen3 */
> +
> +static const struct of_device_id renesas_socs[] __initconst = {
> +#ifdef CONFIG_ARCH_R8A73A4
> + { .compatible = "renesas,r8a73a4", .data = (void *)PRR, },
> +#endif
> +#ifdef CONFIG_ARCH_R8A7740
> + { .compatible = "renesas,r8a7740", .data = (void *)CCCR, },
> +#endif
My preference here would be to list the register address only for
SoCs that are known to need them, while also having .dtb files that
don't have the nodes.
> +static int __init renesas_soc_init(void)
> +{
> + struct soc_device_attribute *soc_dev_attr;
> + const struct of_device_id *match;
> + void __iomem *chipid = NULL;
> + struct soc_device *soc_dev;
> + struct device_node *np;
> + unsigned int product;
> +
> + np = of_find_matching_node_and_match(NULL, renesas_socs, &match);
> + if (!np)
> + return -ENODEV;
> +
> + of_node_put(np);
> +
> + /* Try PRR first, then CCCR, then hardcoded fallback */
> + np = of_find_compatible_node(NULL, NULL, "renesas,prr");
> + if (!np)
> + np = of_find_compatible_node(NULL, NULL, "renesas,cccr");
> + if (np) {
> + chipid = of_iomap(np, 0);
> + of_node_put(np);
> + } else if (match->data) {
> + chipid = ioremap((uintptr_t)match->data, 4);
> + }
> + if (!chipid)
>
Here, I'd turn the order around and look for the DT nodes of the
devices first. Only if they are not found, look at the compatible
string of the root node. No need to search for a node though,
you know which one it is when you look for a compatible =
"renesas,r8a73a4".
Arnd
^ permalink raw reply
* [PATCH v2 0/7] soc: renesas: Identify SoC and register with the SoC bus
From: Arnd Bergmann @ 2016-11-09 16:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAMuHMdUmpMpizZpq1V-sLA8Cf2q5oOgOVxGOvKXqTHvn+Mj7Tg@mail.gmail.com>
On Wednesday, November 9, 2016 2:34:33 PM CET Geert Uytterhoeven wrote:
> >
> > And Samsung.
> > Shall I create the immutable branch now?
>
> Arnd: are you happy with the new patches and changes?
I still had some comments for patch 7, but that shouldn't stop
you from creating a branch for the first three so everyone can
build on top of that.
Arnd
^ permalink raw reply
* [PATCH V3 0/8] IOMMU probe deferral support
From: Will Deacon @ 2016-11-09 16:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <002001d23a51$ecb01390$c6103ab0$@codeaurora.org>
On Wed, Nov 09, 2016 at 11:54:20AM +0530, Sricharan wrote:
> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> >> index 71ce4b6..a1d0b3c 100644
> >> --- a/drivers/iommu/arm-smmu.c
> >> +++ b/drivers/iommu/arm-smmu.c
> >> @@ -1516,8 +1516,10 @@ static struct iommu_group *arm_smmu_device_group(struct device *dev)
> >> group = smmu->s2crs[idx].group;
> >> }
> >>
> >> - if (group)
> >> + if (group) {
> >> + iommu_group_get_by_id(iommu_group_id(group));
> >> return group;
> >
> >This might as well just be inline, i.e.:
> >
> > return iommu_group_get_by_id(iommu_group_id(group));
> >
> >It's a shame we have to go all round the houses when we have the group
> >right there, but this is probably the most expedient fix. I guess we can
> >extend the API with some sort of iommu_group_get(group) overload in
> >future if we really want to.
> >
>
> ok, i can send this fix separately then. Otherwise, Will was suggesting on the
> other thread that there should probably be a separate API to increment
> the group refcount or get the group from the existing aliasing device.
> As per me adding the api, looks like another option or post the above ?
I think adding a new function to the API is the way to go -- having code
like what you propose above littered around the drivers is hard to read and
pretty error-prone, since it looks like a NOP to people who aren't already
thinking about ref counts.
Will
^ permalink raw reply
* Summary of LPC guest MSI discussion in Santa Fe
From: Will Deacon @ 2016-11-09 17:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <58228F71.6020108@redhat.com>
On Tue, Nov 08, 2016 at 09:52:33PM -0500, Don Dutile wrote:
> On 11/08/2016 06:35 PM, Alex Williamson wrote:
> >On Tue, 8 Nov 2016 21:29:22 +0100
> >Christoffer Dall <christoffer.dall@linaro.org> wrote:
> >>Is my understanding correct, that you need to tell userspace about the
> >>location of the doorbell (in the IOVA space) in case (2), because even
> >>though the configuration of the device is handled by the (host) kernel
> >>through trapping of the BARs, we have to avoid the VFIO user programming
> >>the device to create other DMA transactions to this particular address,
> >>since that will obviously conflict and either not produce the desired
> >>DMA transactions or result in unintended weird interrupts?
Yes, that's the crux of the issue.
> >Correct, if the MSI doorbell IOVA range overlaps RAM in the VM, then
> >it's potentially a DMA target and we'll get bogus data on DMA read from
> >the device, and lose data and potentially trigger spurious interrupts on
> >DMA write from the device. Thanks,
> >
> That's b/c the MSI doorbells are not positioned *above* the SMMU, i.e.,
> they address match before the SMMU checks are done. if
> all DMA addrs had to go through SMMU first, then the DMA access could
> be ignored/rejected.
That's actually not true :( The SMMU can't generally distinguish between MSI
writes and DMA writes, so it would just see a write transaction to the
doorbell address, regardless of how it was generated by the endpoint.
Will
^ permalink raw reply
* [PATCH] arm64: percpu: kill off final ACCESS_ONCE() uses
From: Catalin Marinas @ 2016-11-09 17:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478283426-6649-1-git-send-email-mark.rutland@arm.com>
On Fri, Nov 04, 2016 at 06:17:06PM +0000, Mark Rutland wrote:
> For several reasons it is preferable to use {READ,WRITE}_ONCE() rather than
> ACCESS_ONCE(). For example, these handle aggregate types, result in shorter
> source code, and better document the intended access (which may be useful for
> instrumentation features such as the upcoming KTSAN).
>
> Over a number of patches, most uses of ACCESS_ONCE() in arch/arm64 have been
> migrated to {READ,WRITE}_ONCE(). For consistency, and the above reasons, this
> patch migrates the final remaining uses.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Will Deacon <will.deacon@arm.com>
I'll queue this for 4.10. Thanks.
--
Catalin
^ permalink raw reply
* [PATCH v2 0/7] soc: renesas: Identify SoC and register with the SoC bus
From: Geert Uytterhoeven @ 2016-11-09 17:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1626072.dpWPnWgGl9@wuerfel>
Hi Arnd,
On Wed, Nov 9, 2016 at 5:56 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday, November 9, 2016 2:34:33 PM CET Geert Uytterhoeven wrote:
>> > And Samsung.
>> > Shall I create the immutable branch now?
>>
>> Arnd: are you happy with the new patches and changes?
>
> I still had some comments for patch 7, but that shouldn't stop
> you from creating a branch for the first three so everyone can
> build on top of that.
Thanks!
What about patch [4/7]?
Haven't you received it? Your address was in the To-line for all 7 patches.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH v15 4/4] arm: dts: mt2701: Use real clock for UARTs
From: Matthias Brugger @ 2016-11-09 17:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478245388-1412-5-git-send-email-erin.lo@mediatek.com>
On 11/04/2016 08:43 AM, Erin Lo wrote:
> We used to use a fixed rate clock for the UARTs. Now that we have clock
> support we can associate the correct clocks to the UARTs and drop the
> 26MHz fixed rate UART clock.
>
> Signed-off-by: Erin Lo <erin.lo@mediatek.com>
Applied, thanks.
> ---
> arch/arm/boot/dts/mt2701.dtsi | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/boot/dts/mt2701.dtsi b/arch/arm/boot/dts/mt2701.dtsi
> index c9a8dbf..7eab6f4 100644
> --- a/arch/arm/boot/dts/mt2701.dtsi
> +++ b/arch/arm/boot/dts/mt2701.dtsi
> @@ -73,12 +73,6 @@
> #clock-cells = <0>;
> };
>
> - uart_clk: dummy26m {
> - compatible = "fixed-clock";
> - clock-frequency = <26000000>;
> - #clock-cells = <0>;
> - };
> -
> clk26m: oscillator at 0 {
> compatible = "fixed-clock";
> #clock-cells = <0>;
> @@ -186,7 +180,8 @@
> "mediatek,mt6577-uart";
> reg = <0 0x11002000 0 0x400>;
> interrupts = <GIC_SPI 51 IRQ_TYPE_LEVEL_LOW>;
> - clocks = <&uart_clk>;
> + clocks = <&pericfg CLK_PERI_UART0_SEL>, <&pericfg CLK_PERI_UART0>;
> + clock-names = "baud", "bus";
> status = "disabled";
> };
>
> @@ -195,7 +190,8 @@
> "mediatek,mt6577-uart";
> reg = <0 0x11003000 0 0x400>;
> interrupts = <GIC_SPI 52 IRQ_TYPE_LEVEL_LOW>;
> - clocks = <&uart_clk>;
> + clocks = <&pericfg CLK_PERI_UART1_SEL>, <&pericfg CLK_PERI_UART1>;
> + clock-names = "baud", "bus";
> status = "disabled";
> };
>
> @@ -204,7 +200,8 @@
> "mediatek,mt6577-uart";
> reg = <0 0x11004000 0 0x400>;
> interrupts = <GIC_SPI 53 IRQ_TYPE_LEVEL_LOW>;
> - clocks = <&uart_clk>;
> + clocks = <&pericfg CLK_PERI_UART2_SEL>, <&pericfg CLK_PERI_UART2>;
> + clock-names = "baud", "bus";
> status = "disabled";
> };
>
> @@ -213,7 +210,8 @@
> "mediatek,mt6577-uart";
> reg = <0 0x11005000 0 0x400>;
> interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_LOW>;
> - clocks = <&uart_clk>;
> + clocks = <&pericfg CLK_PERI_UART3_SEL>, <&pericfg CLK_PERI_UART3>;
> + clock-names = "baud", "bus";
> status = "disabled";
> };
> };
>
^ permalink raw reply
* [PATCH 1/5] iommu: Allow taking a reference on a group directly
From: Will Deacon @ 2016-11-09 17:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <3922e1f14d8ecb50440b2d9b0d1123f3c9307fc5.1478695557.git.robin.murphy@arm.com>
On Wed, Nov 09, 2016 at 12:47:24PM +0000, Robin Murphy wrote:
> iommu_group_get_for_dev() expects that the IOMMU driver's device_group
> callback return a group with a reference held for the given device.
> Whilst allocating a new group is fine, and pci_device_group() correctly
> handles reusing an existing group, there is no general means for IOMMU
> drivers doing their own group lookup to take additional references on an
> existing group pointer without having to also store device pointers or
> resort to elaborate trickery.
>
> Add an IOMMU-driver-specific function to fill the hole.
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> drivers/iommu/iommu.c | 14 ++++++++++++++
> include/linux/iommu.h | 1 +
> 2 files changed, 15 insertions(+)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 9a2f1960873b..b0b052bc6bb5 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -552,6 +552,20 @@ struct iommu_group *iommu_group_get(struct device *dev)
> EXPORT_SYMBOL_GPL(iommu_group_get);
>
> /**
> + * __iommu_group_get - Increment reference on a group
> + * @group: the group to use, must not be NULL
> + *
> + * This function may be called by internal iommu driver group management
> + * when the context of a struct device pointer is not available. It is
> + * not for general use. Returns the given group for convenience.
> + */
> +struct iommu_group *__iommu_group_get(struct iommu_group *group)
> +{
> + kobject_get(group->devices_kobj);
> + return group;
> +}
This probably either wants sticking in a header or exporting to modules.
That said, why do we need the underscores and the comment about internal
group management? That's pretty much already the case for iommu_group_get.
Of course, removing the underscores gives you a naming conflict, but we
could just call it something like "iommu_group_get_ref".
Will
^ permalink raw reply
* [PATCH] fpga zynq: Check the bitstream for validity
From: Mike Looijmans @ 2016-11-09 17:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161109155651.GA15076@obsidianresearch.com>
?On 09-11-16 16:56, Jason Gunthorpe wrote:
> On Wed, Nov 09, 2016 at 03:21:52PM +0100, Mike Looijmans wrote:
>> I think the basic idea behind the commit is flawed to begin with and the
>> patch should be discarded completely. The same discussion was done for the
>> Xilinx FPGA manager driver, which resulted in the decision that the tooling
>> should create a proper file.
>
> That hasn't changed at all, this just produces a clear and obvious
> message about what is wrong instead of 'timed out'.
>
> Remember, again, the Xilinx tools do not produce an acceptable
> bitstream file by default. You need to do very strange and special
> things to get something acceptable to this driver. It is a very easy
> mistake to make and hard to track down if you don't already know these
> details.
>
>> This driver should either become obsolete or at least move into the
>> same direction as the FPGA manager rather than away from that.
>
> I don't understand what you are talking about here, this is a fpga mgr
> driver already, and is consistent with the FPGA manger - the auto
> detect stuff was removed a while ago and isn't coming back.
That's exactly what I mean - the code was kept simple.
> It is perfectly reasonable for fpga manager drivers to pre-validate
> the proposed bitstream, IMHO all of the drivers should try and do
> that step.
>
> Remember, it is deeply unlikely but sending garbage to an FPGA could
> damage it.
Then what's the purpose of pre-validation? Sending valid data should be
the normal case, not the exception.
>> Besides political arguments, there's a more pressing technical argument
>> against this theck as well: The whole check is pointless since the hardware
>> itself already verifies the validity of the stream.
>
> The purpose of the check is not to protect the hardware. The check is
> to help the user provide the correct input because the hardware
> provides no feedback at all on what is wrong with the input.
>
> And again, the out-of-tree Xilinx driver accepted files that this
> driver does not, so having a clear and understandable message is going
> to be very important for users.
Then just create a "validate my bitstream" tool.
I wrote a Python script to do what Xilinx refused to do years ago:
https://github.com/topic-embedded-products/meta-topic/blob/master/recipes-bsp/fpga/fpga-bit-to-bin/fpga-bit-to-bin.py
So apparently it wasn't hard to figure out what to do.
>> doesn't have any effect, the hardware will discard it. There's no reason to
>> waste CPU cycles duplicating this check in software.
>
> In the typical success case we are talking about 5 32 bit compares,
> which isn't even worth considering.
I'm mostly against the complication of the code. The code is more
complex, and that's bad. It's firmware loading code, and it need not be
aware of exactly what it is doing. I did not see any checks like this in
other firmware loading code I've come across.
What you're creating here will require active maintenance.
There are already 7007 and 7012 devices which aren't in your list so the
driver will refuse to load them until someone fills in the IDs.
The bitstream format is actually proprietary and undocumented. Any
"checks" in code are likely based on guesswork and reverse engineering.
We also use partial reprogramming a lot. Did you test that? On all
devices? And we're actually planning to go beyond that. Maybe I'll be
providing parts of the data through ICAP and some through PCAP, that
might even work, but the driver would block it for no apparent reason.
--
Mike Looijmans
Kind regards,
Mike Looijmans
System Expert
TOPIC Products
Materiaalweg 4, NL-5681 RJ Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans at topicproducts.com
Website: www.topicproducts.com
Please consider the environment before printing this e-mail
^ permalink raw reply
* [PATCH V2 2/5] arm64: Allow hw watchpoint at varied offset from base address
From: Pratyush Anand @ 2016-11-09 17:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161108032658.GB20591@arm.com>
Hi Will,
Thanks a lot for your review.
On Tuesday 08 November 2016 08:56 AM, Will Deacon wrote:
> Hi Pratyush,
>
> On Thu, Oct 20, 2016 at 11:18:14AM +0530, Pratyush Anand wrote:
>> ARM64 hardware supports watchpoint at any double word aligned address.
>> However, it can select any consecutive bytes from offset 0 to 7 from that
>> base address. For example, if base address is programmed as 0x420030 and
>> byte select is 0x1C, then access of 0x420032,0x420033 and 0x420034 will
>> generate a watchpoint exception.
>>
>> Currently, we do not have such modularity. We can only program byte,
>> halfword, word and double word access exception from any base address.
>>
>> This patch adds support to overcome above limitations.
>>
>> Signed-off-by: Pratyush Anand <panand@redhat.com>
>> ---
>> arch/arm64/include/asm/hw_breakpoint.h | 2 +-
>> arch/arm64/kernel/hw_breakpoint.c | 45 ++++++++++++++++------------------
>> arch/arm64/kernel/ptrace.c | 5 ++--
>> 3 files changed, 25 insertions(+), 27 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
>> index 115ea2a64520..4f4e58bee9bc 100644
>> --- a/arch/arm64/include/asm/hw_breakpoint.h
>> +++ b/arch/arm64/include/asm/hw_breakpoint.h
>> @@ -118,7 +118,7 @@ struct perf_event;
>> struct pmu;
>>
>> extern int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
>> - int *gen_len, int *gen_type);
>> + int *gen_len, int *gen_type, int *offset);
>> extern int arch_check_bp_in_kernelspace(struct perf_event *bp);
>> extern int arch_validate_hwbkpt_settings(struct perf_event *bp);
>> extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
>> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
>> index 26a6bf77d272..3c2b96803eba 100644
>> --- a/arch/arm64/kernel/hw_breakpoint.c
>> +++ b/arch/arm64/kernel/hw_breakpoint.c
>> @@ -349,7 +349,7 @@ int arch_check_bp_in_kernelspace(struct perf_event *bp)
>> * to generic breakpoint descriptions.
>> */
>> int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
>> - int *gen_len, int *gen_type)
>> + int *gen_len, int *gen_type, int *offset)
>> {
>> /* Type */
>> switch (ctrl.type) {
>> @@ -369,8 +369,10 @@ int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
>> return -EINVAL;
>> }
>>
>> + *offset = ffs(ctrl.len) - 1;
>
> Do we want to fail the length == 0 case early? If you do add that check,
> then you can use __ffs here instead.
Yes, I think, your point can be taken.
>
>> /* Len */
>> - switch (ctrl.len) {
>> + switch (ctrl.len >> *offset) {
>> case ARM_BREAKPOINT_LEN_1:
>> *gen_len = HW_BREAKPOINT_LEN_1;
>> break;
>> @@ -517,18 +519,17 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
>> default:
>> return -EINVAL;
>> }
>> -
>> - info->address &= ~alignment_mask;
>> - info->ctrl.len <<= offset;
>> } else {
>> if (info->ctrl.type == ARM_BREAKPOINT_EXECUTE)
>> alignment_mask = 0x3;
>> else
>> alignment_mask = 0x7;
>> - if (info->address & alignment_mask)
>> - return -EINVAL;
>> + offset = info->address & alignment_mask;
>> }
>>
>> + info->address &= ~alignment_mask;
>> + info->ctrl.len <<= offset;
>
> Urgh, I really hate all this converting to and fro to keep perf happy.
> FWIW, I'm at the point where I would seriously consider ripping out the
> hw_breakpoint code and replacing it with a ptrace-specific backend so we
> just drop all this crap. The only people that seem to use the perf interface
> are those running the (failing) selftests. Given that we have to have
> a bloody thread switch notifier *anyway*, all perf seems to give us is
> complexity and restrictions.
Yes, I agree.
>
>> /*
>> * Disallow per-task kernel breakpoints since these would
>> * complicate the stepping code.
>> @@ -665,8 +666,8 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr,
>> struct pt_regs *regs)
>> {
>> int i, step = 0, *kernel_step, access;
>> - u32 ctrl_reg;
>> - u64 val, alignment_mask;
>> + u32 ctrl_reg, lens, lene;
>> + u64 val;
>> struct perf_event *wp, **slots;
>> struct debug_info *debug_info;
>> struct arch_hw_breakpoint *info;
>> @@ -684,25 +685,21 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr,
>> goto unlock;
>>
>> info = counter_arch_bp(wp);
>> - /* AArch32 watchpoints are either 4 or 8 bytes aligned. */
>> - if (is_compat_task()) {
>> - if (info->ctrl.len == ARM_BREAKPOINT_LEN_8)
>> - alignment_mask = 0x7;
>> - else
>> - alignment_mask = 0x3;
>> - } else {
>> - alignment_mask = 0x7;
>> - }
>>
>> - /* Check if the watchpoint value matches. */
>> + /* Check if the watchpoint value and byte select match. */
>> val = read_wb_reg(AARCH64_DBG_REG_WVR, i);
>> - if (val != (addr & ~alignment_mask))
>> - goto unlock;
>> -
>> - /* Possible match, check the byte address select to confirm. */
>> ctrl_reg = read_wb_reg(AARCH64_DBG_REG_WCR, i);
>> decode_ctrl_reg(ctrl_reg, &ctrl);
>> - if (!((1 << (addr & alignment_mask)) & ctrl.len))
>> + lens = ffs(ctrl.len) - 1;
>> + lene = fls(ctrl.len) - 1;
>
> Again, you can use the '__' variants to avoid the '- 1'.
Ok.
>
>> + /*
>> + * FIXME: reported address can be anywhere between "the
>> + * lowest address accessed by the memory access that
>> + * triggered the watchpoint" and "the highest watchpointed
>> + * address accessed by the memory access". So, it may not
>> + * lie in the interval of watchpoint address range.
>> + */
>> + if (addr < val + lens || addr > val + lene)
>> goto unlock;
>>
>> /*
>> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
>> index e0c81da60f76..0eb366a94382 100644
>> --- a/arch/arm64/kernel/ptrace.c
>> +++ b/arch/arm64/kernel/ptrace.c
>> @@ -327,13 +327,13 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned int note_type,
>> struct arch_hw_breakpoint_ctrl ctrl,
>> struct perf_event_attr *attr)
>> {
>> - int err, len, type, disabled = !ctrl.enabled;
>> + int err, len, type, offset, disabled = !ctrl.enabled;
>>
>> attr->disabled = disabled;
>> if (disabled)
>> return 0;
>>
>> - err = arch_bp_generic_fields(ctrl, &len, &type);
>> + err = arch_bp_generic_fields(ctrl, &len, &type, &offset);
>> if (err)
>> return err;
>>
>> @@ -352,6 +352,7 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned int note_type,
>>
>> attr->bp_len = len;
>> attr->bp_type = type;
>> + attr->bp_addr += offset;
>
> That's going to look pretty bizarre from userspace if it decides to read
> back the address registers to find that they've mysteriously changed.
>
> Perhaps ptrace_hbp_get_addr needs to retrieve the address from the
> arch_hw_breakpoint, like we do for the ctrl register. What do you think?
..and that should help...I will give it a try and will test before next
revision.
~Pratyush
^ 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