* [PATCH AUTOSEL 4.14 21/39] scsi: ibmvscsi: Fix WARN_ON during event pool release
From: Sasha Levin @ 2020-05-14 18:54 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Sasha Levin, Tyrel Datwyler, linuxppc-dev, linux-scsi,
Martin K . Petersen
In-Reply-To: <20200514185456.21060-1-sashal@kernel.org>
From: Tyrel Datwyler <tyreld@linux.ibm.com>
[ Upstream commit b36522150e5b85045f868768d46fbaaa034174b2 ]
While removing an ibmvscsi client adapter a WARN_ON like the following is
seen in the kernel log:
drmgr: drmgr: -r -c slot -s U9080.M9S.783AEC8-V11-C11 -w 5 -d 1
WARNING: CPU: 9 PID: 24062 at ../kernel/dma/mapping.c:311 dma_free_attrs+0x78/0x110
Supported: No, Unreleased kernel
CPU: 9 PID: 24062 Comm: drmgr Kdump: loaded Tainted: G X 5.3.18-12-default
NIP: c0000000001fa758 LR: c0000000001fa744 CTR: c0000000001fa6e0
REGS: c0000002173375d0 TRAP: 0700 Tainted: G X (5.3.18-12-default)
MSR: 8000000000029033 <SF,EE,ME,IR,DR,RI,LE> CR: 28088282 XER: 20000000
CFAR: c0000000001fbf0c IRQMASK: 1
GPR00: c0000000001fa744 c000000217337860 c00000000161ab00 0000000000000000
GPR04: 0000000000000000 c000011e12250000 0000000018010000 0000000000000000
GPR08: 0000000000000000 0000000000000001 0000000000000001 c0080000190f4fa8
GPR12: c0000000001fa6e0 c000000007fc2a00 0000000000000000 0000000000000000
GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR24: 000000011420e310 0000000000000000 0000000000000000 0000000018010000
GPR28: c00000000159de50 c000011e12250000 0000000000006600 c000011e5c994848
NIP [c0000000001fa758] dma_free_attrs+0x78/0x110
LR [c0000000001fa744] dma_free_attrs+0x64/0x110
Call Trace:
[c000000217337860] [000000011420e310] 0x11420e310 (unreliable)
[c0000002173378b0] [c0080000190f0280] release_event_pool+0xd8/0x120 [ibmvscsi]
[c000000217337930] [c0080000190f3f74] ibmvscsi_remove+0x6c/0x160 [ibmvscsi]
[c000000217337960] [c0000000000f3cac] vio_bus_remove+0x5c/0x100
[c0000002173379a0] [c00000000087a0a4] device_release_driver_internal+0x154/0x280
[c0000002173379e0] [c0000000008777cc] bus_remove_device+0x11c/0x220
[c000000217337a60] [c000000000870fc4] device_del+0x1c4/0x470
[c000000217337b10] [c0000000008712a0] device_unregister+0x30/0xa0
[c000000217337b80] [c0000000000f39ec] vio_unregister_device+0x2c/0x60
[c000000217337bb0] [c00800001a1d0964] dlpar_remove_slot+0x14c/0x250 [rpadlpar_io]
[c000000217337c50] [c00800001a1d0bcc] remove_slot_store+0xa4/0x110 [rpadlpar_io]
[c000000217337cd0] [c000000000c091a0] kobj_attr_store+0x30/0x50
[c000000217337cf0] [c00000000057c934] sysfs_kf_write+0x64/0x90
[c000000217337d10] [c00000000057be10] kernfs_fop_write+0x1b0/0x290
[c000000217337d60] [c000000000488c4c] __vfs_write+0x3c/0x70
[c000000217337d80] [c00000000048c648] vfs_write+0xd8/0x260
[c000000217337dd0] [c00000000048ca8c] ksys_write+0xdc/0x130
[c000000217337e20] [c00000000000b488] system_call+0x5c/0x70
Instruction dump:
7c840074 f8010010 f821ffb1 20840040 eb830218 7c8407b4 48002019 60000000
2fa30000 409e003c 892d0988 792907e0 <0b090000> 2fbd0000 419e0028 2fbc0000
---[ end trace 5955b3c0cc079942 ]---
rpadlpar_io: slot U9080.M9S.783AEC8-V11-C11 removed
This is tripped as a result of irqs being disabled during the call to
dma_free_coherent() by release_event_pool(). At this point in the code path
we have quiesced the adapter and it is overly paranoid to be holding the
host lock.
[mkp: fixed build warning reported by sfr]
Link: https://lore.kernel.org/r/1588027793-17952-1-git-send-email-tyreld@linux.ibm.com
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/scsi/ibmvscsi/ibmvscsi.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 07c23bbd968c5..83645a1c6f82e 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -2299,16 +2299,12 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const struct vio_device_id *id)
static int ibmvscsi_remove(struct vio_dev *vdev)
{
struct ibmvscsi_host_data *hostdata = dev_get_drvdata(&vdev->dev);
- unsigned long flags;
srp_remove_host(hostdata->host);
scsi_remove_host(hostdata->host);
purge_requests(hostdata, DID_ERROR);
-
- spin_lock_irqsave(hostdata->host->host_lock, flags);
release_event_pool(&hostdata->pool, hostdata);
- spin_unlock_irqrestore(hostdata->host->host_lock, flags);
ibmvscsi_release_crq_queue(&hostdata->queue, hostdata,
max_events);
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v3 2/2] powerpc/rtas: Implement reentrant rtas call
From: Nathan Lynch @ 2020-05-14 18:58 UTC (permalink / raw)
To: Leonardo Bras
Cc: Leonardo Bras, Gautham R. Shenoy, Greg Kroah-Hartman,
linux-kernel, Nicholas Piggin, Paul Mackerras, Nadav Amit,
Thomas Gleixner, linuxppc-dev, Allison Randal
In-Reply-To: <20200514011245.127174-3-leobras.c@gmail.com>
Hi,
Leonardo Bras <leobras.c@gmail.com> writes:
> +/**
> + * rtas_call_reentrant() - Used for reentrant rtas calls
> + * @token: Token for desired reentrant RTAS call
> + * @nargs: Number of Input Parameters
> + * @nret: Number of Output Parameters
> + * @outputs: Array of outputs
> + * @...: Inputs for desired RTAS call
> + *
> + * According to LoPAR documentation, only "ibm,int-on", "ibm,int-off",
> + * "ibm,get-xive" and "ibm,set-xive" are currently reentrant.
> + * Reentrant calls need their own rtas_args buffer, so not using rtas.args, but
> + * PACA one instead.
> + *
> + * Return: -1 on error,
> + * First output value of RTAS call if (nret > 0),
> + * 0 otherwise,
> + */
> +
> +int rtas_call_reentrant(int token, int nargs, int nret, int *outputs, ...)
> +{
> + va_list list;
> + struct rtas_args *args;
> + int i;
> +
> + if (!rtas.entry || token == RTAS_UNKNOWN_SERVICE)
> + return -1;
> +
> + /* We use the per-cpu (PACA) rtas args buffer */
> + args = &local_paca->reentrant_args;
> +
> + va_start(list, outputs);
> + va_rtas_call_unlocked(args, token, nargs, nret, list);
> + va_end(list);
> +
> + if (nret > 1 && outputs)
> + for (i = 0; i < nret - 1; ++i)
> + outputs[i] = be32_to_cpu(args->rets[i + 1]);
Doesn't this need to be more careful about preemption and exceptions?
I.e. the args structure in the paca needs to be protected from
concurrent use somehow, like any per-cpu data structure.
local_irq_save/restore while accessing local_paca->reentrant_args here
would be sufficient I think?
^ permalink raw reply
* Re: [PATCH 1/1] powerpc/rtas: Implement reentrant rtas call
From: Nathan Lynch @ 2020-05-14 19:04 UTC (permalink / raw)
To: Leonardo Bras
Cc: Gautham R. Shenoy, Greg Kroah-Hartman, linux-kernel,
Nicholas Piggin, Paul Mackerras, Nadav Amit, Thomas Gleixner,
linuxppc-dev, Allison Randal
In-Reply-To: <ba97d52df60ac9c4a4afc2c03121a8c263aa5a15.camel@linux.ibm.com>
Hi Leonardo,
Leonardo Bras <leonardo@linux.ibm.com> writes:
> Hello Nathan, thanks for the feedback!
>
> On Fri, 2020-04-10 at 14:28 -0500, Nathan Lynch wrote:
>> Leonardo Bras <leonardo@linux.ibm.com> writes:
>> > Implement rtas_call_reentrant() for reentrant rtas-calls:
>> > "ibm,int-on", "ibm,int-off",ibm,get-xive" and "ibm,set-xive".
>> >
>> > On LoPAPR Version 1.1 (March 24, 2016), from 7.3.10.1 to 7.3.10.4,
>> > items 2 and 3 say:
>> >
>> > 2 - For the PowerPC External Interrupt option: The * call must be
>> > reentrant to the number of processors on the platform.
>> > 3 - For the PowerPC External Interrupt option: The * argument call
>> > buffer for each simultaneous call must be physically unique.
>> >
>> > So, these rtas-calls can be called in a lockless way, if using
>> > a different buffer for each call.
>> >
>
>> From the language in the spec it's clear that these calls are intended
>> to be reentrant with respect to themselves, but it's less clear to me
>> that they are safe to call simultaneously with respect to each other or
>> arbitrary other RTAS methods.
>
> In my viewpoint, being reentrant to themselves, without being reentrant
> to others would be very difficult to do, considering the way the
> rtas_call is crafted to work.
>
> I mean, I have no experience in rtas code, it's my viewpoint. In my
> thoughts there is something like this:
>
> common_path -> selects function by token -> reentrant function
> |-> non-reentrant function
>
> If there is one function that is reentrant, it means the common_path
> and function selection by token would need to be reentrant too.
I checked with partition firmware development and these calls can be
used concurrently with arbitrary other RTAS calls, which confirms your
interpretation. Thanks for bearing with me.
^ permalink raw reply
* Re: [PATCH 1/1] powerpc/rtas: Implement reentrant rtas call
From: Leonardo Bras @ 2020-05-14 20:22 UTC (permalink / raw)
To: Nathan Lynch
Cc: Gautham R. Shenoy, Greg Kroah-Hartman, linux-kernel,
Nicholas Piggin, Paul Mackerras, Nadav Amit, Thomas Gleixner,
linuxppc-dev, Allison Randal
In-Reply-To: <875zcy2v8o.fsf@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 365 bytes --]
On Thu, 2020-05-14 at 14:04 -0500, Nathan Lynch wrote:
> I checked with partition firmware development and these calls can be
> used concurrently with arbitrary other RTAS calls, which confirms your
> interpretation. Thanks for bearing with me.
I was not aware of how I could get this information.
It was of great help!
Thank you for your contribution!
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 862 bytes --]
^ permalink raw reply
* Re: [PATCH] tty: hvc: Fix data abort due to race in hvc_open
From: rananta @ 2020-05-14 23:22 UTC (permalink / raw)
To: Greg KH; +Cc: linuxppc-dev, andrew, Jiri Slaby, linux-kernel
In-Reply-To: <20200513070403.GB764901@kroah.com>
On 2020-05-13 00:04, Greg KH wrote:
> On Tue, May 12, 2020 at 02:39:50PM -0700, rananta@codeaurora.org wrote:
>> On 2020-05-12 01:25, Greg KH wrote:
>> > On Tue, May 12, 2020 at 09:22:15AM +0200, Jiri Slaby wrote:
>> > > On 11. 05. 20, 9:39, Greg KH wrote:
>> > > > On Mon, May 11, 2020 at 12:23:58AM -0700, rananta@codeaurora.org wrote:
>> > > >> On 2020-05-09 23:48, Greg KH wrote:
>> > > >>> On Sat, May 09, 2020 at 06:30:56PM -0700, rananta@codeaurora.org wrote:
>> > > >>>> On 2020-05-06 02:48, Greg KH wrote:
>> > > >>>>> On Mon, Apr 27, 2020 at 08:26:01PM -0700, Raghavendra Rao Ananta wrote:
>> > > >>>>>> Potentially, hvc_open() can be called in parallel when two tasks calls
>> > > >>>>>> open() on /dev/hvcX. In such a scenario, if the
>> > > >>>>>> hp->ops->notifier_add()
>> > > >>>>>> callback in the function fails, where it sets the tty->driver_data to
>> > > >>>>>> NULL, the parallel hvc_open() can see this NULL and cause a memory
>> > > >>>>>> abort.
>> > > >>>>>> Hence, serialize hvc_open and check if tty->private_data is NULL
>> > > >>>>>> before
>> > > >>>>>> proceeding ahead.
>> > > >>>>>>
>> > > >>>>>> The issue can be easily reproduced by launching two tasks
>> > > >>>>>> simultaneously
>> > > >>>>>> that does nothing but open() and close() on /dev/hvcX.
>> > > >>>>>> For example:
>> > > >>>>>> $ ./simple_open_close /dev/hvc0 & ./simple_open_close /dev/hvc0 &
>> > > >>>>>>
>> > > >>>>>> Signed-off-by: Raghavendra Rao Ananta <rananta@codeaurora.org>
>> > > >>>>>> ---
>> > > >>>>>> drivers/tty/hvc/hvc_console.c | 16 ++++++++++++++--
>> > > >>>>>> 1 file changed, 14 insertions(+), 2 deletions(-)
>> > > >>>>>>
>> > > >>>>>> diff --git a/drivers/tty/hvc/hvc_console.c
>> > > >>>>>> b/drivers/tty/hvc/hvc_console.c
>> > > >>>>>> index 436cc51c92c3..ebe26fe5ac09 100644
>> > > >>>>>> --- a/drivers/tty/hvc/hvc_console.c
>> > > >>>>>> +++ b/drivers/tty/hvc/hvc_console.c
>> > > >>>>>> @@ -75,6 +75,8 @@ static LIST_HEAD(hvc_structs);
>> > > >>>>>> */
>> > > >>>>>> static DEFINE_MUTEX(hvc_structs_mutex);
>> > > >>>>>>
>> > > >>>>>> +/* Mutex to serialize hvc_open */
>> > > >>>>>> +static DEFINE_MUTEX(hvc_open_mutex);
>> > > >>>>>> /*
>> > > >>>>>> * This value is used to assign a tty->index value to a hvc_struct
>> > > >>>>>> based
>> > > >>>>>> * upon order of exposure via hvc_probe(), when we can not match it
>> > > >>>>>> to
>> > > >>>>>> @@ -346,16 +348,24 @@ static int hvc_install(struct tty_driver
>> > > >>>>>> *driver, struct tty_struct *tty)
>> > > >>>>>> */
>> > > >>>>>> static int hvc_open(struct tty_struct *tty, struct file * filp)
>> > > >>>>>> {
>> > > >>>>>> - struct hvc_struct *hp = tty->driver_data;
>> > > >>>>>> + struct hvc_struct *hp;
>> > > >>>>>> unsigned long flags;
>> > > >>>>>> int rc = 0;
>> > > >>>>>>
>> > > >>>>>> + mutex_lock(&hvc_open_mutex);
>> > > >>>>>> +
>> > > >>>>>> + hp = tty->driver_data;
>> > > >>>>>> + if (!hp) {
>> > > >>>>>> + rc = -EIO;
>> > > >>>>>> + goto out;
>> > > >>>>>> + }
>> > > >>>>>> +
>> > > >>>>>> spin_lock_irqsave(&hp->port.lock, flags);
>> > > >>>>>> /* Check and then increment for fast path open. */
>> > > >>>>>> if (hp->port.count++ > 0) {
>> > > >>>>>> spin_unlock_irqrestore(&hp->port.lock, flags);
>> > > >>>>>> hvc_kick();
>> > > >>>>>> - return 0;
>> > > >>>>>> + goto out;
>> > > >>>>>> } /* else count == 0 */
>> > > >>>>>> spin_unlock_irqrestore(&hp->port.lock, flags);
>> > > >>>>>
>> > > >>>>> Wait, why isn't this driver just calling tty_port_open() instead of
>> > > >>>>> trying to open-code all of this?
>> > > >>>>>
>> > > >>>>> Keeping a single mutext for open will not protect it from close, it will
>> > > >>>>> just slow things down a bit. There should already be a tty lock held by
>> > > >>>>> the tty core for open() to keep it from racing things, right?
>> > > >>>> The tty lock should have been held, but not likely across
>> > > >>>> ->install() and
>> > > >>>> ->open() callbacks, thus resulting in a race between hvc_install() and
>> > > >>>> hvc_open(),
>> > > >>>
>> > > >>> How? The tty lock is held in install, and should not conflict with
>> > > >>> open(), otherwise, we would be seeing this happen in all tty drivers,
>> > > >>> right?
>> > > >>>
>> > > >> Well, I was expecting the same, but IIRC, I see that the open() was being
>> > > >> called in parallel for the same device node.
>> > > >
>> > > > So open and install are happening at the same time? And the tty_lock()
>> > > > does not protect the needed fields from being protected properly? If
>> > > > not, what fields are being touched without the lock?
>> > > >
>> > > >> Is it expected that the tty core would allow only one thread to
>> > > >> access the dev-node, while blocking the other, or is it the client
>> > > >> driver's responsibility to handle the exclusiveness?
>> > > >
>> > > > The tty core should handle this correctly, for things that can mess
>> > > > stuff up (like install and open at the same time). A driver should not
>> > > > have to worry about that.
>> > > >
>> > > >>>> where hvc_install() sets a data and the hvc_open() clears it.
>> > > >>>> hvc_open()
>> > > >>>> doesn't
>> > > >>>> check if the data was set to NULL and proceeds.
>> > > >>>
>> > > >>> What data is being set that hvc_open is checking?
>> > > >> hvc_install sets tty->private_data to hp, while hvc_open sets it to NULL (in
>> > > >> one of the paths).
>> > > >
>> > > > I see no use of private_data in drivers/tty/hvc/ so what exactly are you
>> > > > referring to?
>> > >
>> > > He likely means tty->driver_data. And there exactly lays the issue.
>> > >
>> > > commit bdb498c20040616e94b05c31a0ceb3e134b7e829
>> > > Author: Jiri Slaby <jslaby@suse.cz>
>> > > Date: Tue Aug 7 21:48:04 2012 +0200
>> > >
>> > > TTY: hvc_console, add tty install
>> > >
>> > > added hvc_install but did not move 'tty->driver_data = NULL;' from
>> > > hvc_open's fail path to hvc_cleanup.
>> > >
>> > > IOW hvc_open now NULLs tty->driver_data even for another task which
>> > > opened the tty earlier. The same holds for
>> > > "tty_port_tty_set(&hp->port,
>> > > NULL);" there. And actually "tty_port_put(&hp->port);" is also
>> > > incorrect
>> > > for the 2nd task opening the tty.
>> > >
>> > > So, a mutex with tty->driver_data check in open is not definitely the
>> > > way to go. This mess needs to be sorted out properly. Sure, a good
>> > > start
>> > > would be a conversion to tty_port_open. Right after dropping "tty:
>> > > hvc:
>> > > Fix data abort due to race in hvc_open" from tty/tty-next :).
>> >
>> > I've now reverted this commit so we can start from a "clean" place.
>> >
>> > > What I *don't* understand is why hp->ops->notifier_add fails, given
>> > > the
>> > > open does not allow multiple opens anyway?
>> >
>> > I don't understand that either. Raghavendra, can you show a real trace
>> > for this issue that shows this?
>> >
>> Let me know if this helps:
>>
>> [ 265.332900] Unable to handle kernel NULL pointer dereference at
>> virtual
>> address 00000000000000a8
>> [ 265.332920] Mem abort info:
>> [ 265.332934] ESR = 0x96000006
>> [ 265.332950] EC = 0x25: DABT (current EL), IL = 32 bits
>> [ 265.332963] SET = 0, FnV = 0
>> [ 265.332975] EA = 0, S1PTW = 0
>> [ 265.332985] Data abort info:
>> [ 265.332997] ISV = 0, ISS = 0x00000006
>> [ 265.333008] CM = 0, WnR = 0
>> [ 265.333025] user pgtable: 4k pages, 39-bit VAs,
>> pgdp=00000001620f3000
>> [ 265.333038] [00000000000000a8] pgd=00000001620f2003,
>> pud=00000001620f2003, pmd=0000000000000000
>> [ 265.333071] Internal error: Oops: 96000006 [#1] PREEMPT SMP
>> [ 265.333424] CPU: 1 PID: 5653 Comm: stress-ng-dev Tainted: G S
>> W O
>> 5.4.12-g04866e0 #1
>> [ 265.333458] pstate: 80400085 (Nzcv daIf +PAN -UAO)
>> [ 265.333499] pc : _raw_spin_lock_irqsave+0x40/0x7c
>> [ 265.333517] lr : _raw_spin_lock_irqsave+0x38/0x7c
>> [ 265.333530] sp : ffffffc02436ba40
>> [ 265.333542] x29: ffffffc02436ba40 x28: 0000000000020800
>> [ 265.333562] x27: ffffffdfb4046490 x26: ffffff8101b83400
>> [ 265.333580] x25: ffffff80e163ad00 x24: ffffffdfb45c7798
>> [ 265.333598] x23: ffffff8101b83668 x22: ffffffdfb4974000
>> [ 265.333617] x21: 0000000000000001 x20: 00000000000000a8
>> [ 265.333634] x19: 0000000000000000 x18: ffffff80e0b0d460
>> [ 265.333652] x17: 0000000000000000 x16: 0000000001000000
>> [ 265.333670] x15: 0000000001000000 x14: 00000000f8000000
>> [ 265.333688] x13: 0000000000000000 x12: 0000000000000001
>> [ 265.333706] x11: 17f5f16765f64600 x10: 17f5f16765f64600
>> [ 265.333724] x9 : ffffffdfb3444244 x8 : 0000000000000000
>> [ 265.333741] x7 : 0000000000000000 x6 : 0000000000000000
>> [ 265.333759] x5 : 0000000000000000 x4 : 0000000000000002
>> [ 265.333776] x3 : ffffffc02436b9c0 x2 : ffffffdfb40456e0
>> [ 265.333794] x1 : ffffffc02436b9c0 x0 : ffffffdfb3444244
>> [ 265.333812] Call trace:
>> [ 265.333831] _raw_spin_lock_irqsave+0x40/0x7c
>> [ 265.333859] 28$61deaf328f140fd7df47c115ec866fa5+0x28/0x174
>> [ 265.333882] tty_open$86bd494905ebe22944bf63b711173de3+0x3d0/0x584
>> [ 265.333921]
>> chrdev_open$4083aaa799bca8e0e1e0c8dc1947aa96+0x1c4/0x248
>> [ 265.333940] do_dentry_open+0x258/0x3b0
>> [ 265.333956] vfs_open+0x2c/0x38
>> [ 265.333975] path_openat+0x898/0xedc
>> [ 265.333991] do_filp_open+0x78/0x124
>> [ 265.334006] do_sys_open+0x13c/0x298
>> [ 265.334022] __arm64_sys_openat+0x28/0x34
>> [ 265.334044] el0_svc_common+0xb8/0x1b4
>> [ 265.334059] el0_svc_handler+0x6c/0x88
>> [ 265.334079] el0_svc+0x8/0xc
>> [ 265.334110] Code: 52800035 97b9fec7 aa1f03e8 f9800291 (885ffe81)
>> [ 265.334130] ---[ end trace ac90e3099a98e99f ]---
>> [ 265.334146] Kernel panic - not syncing: Fatal exception
>
> Hm, do you have a strace showing the close happening at the same time?
> What about install()?
Yes, I do see the close happening, at which point the issue is not seen.
It's only seen if the second task came in before this close was called.
For this task, as the file was already opened, it doesn't go through
hvc_install.
(I figured adding some logs in the drivers would be helpful than straces
to also include hvc_install)
These are the traces you get when the issue happens:
[ 154.212291] hvc_install called for pid: 666
[ 154.216705] hvc_open called for pid: 666
[ 154.233657] hvc_open: request_irq failed with rc -22.
[ 154.238934] hvc_open called for pid: 678
[ 154.243012] Unable to handle kernel NULL pointer dereference at
virtual address 00000000000000c4
# hvc_install isn't called for pid: 678 as the file wasn't closed yet.
And these are the traces we get when things are normal:
[ 76.318861] hvc_install called for pid: 537
[ 76.323240] hvc_open called for pid: 537
[ 76.340177] hvc_open: request_irq failed with rc -22.
[ 76.345384] hvc_close called for pid: 537
[ 76.349555] hvc_install called for pid: 538
[ 76.353921] hvc_open called for pid: 538
# hvc_open for the second task (pid: 538) seems to be fine here as the
file was closed prior to the second task tried to open the file.
>
> And what line in hvc_open() does that offset correspond to?
It's the point where it tries to acquire the spinlock for the first
time: spin_lock_irqsave(&hp->port.lock, flags);
>
> thanks,
>
> greg k-h
Thank you.
Raghavendra
^ permalink raw reply
* Re: [PATCH v3 2/2] powerpc/rtas: Implement reentrant rtas call
From: Leonardo Bras @ 2020-05-14 23:28 UTC (permalink / raw)
To: Nathan Lynch
Cc: Gautham R. Shenoy, Greg Kroah-Hartman, linux-kernel,
Nicholas Piggin, Paul Mackerras, Nadav Amit, Thomas Gleixner,
linuxppc-dev, Allison Randal
In-Reply-To: <878shu2vjp.fsf@linux.ibm.com>
Hello Nathan,
On Thu, 2020-05-14 at 13:58 -0500, Nathan Lynch wrote:
> Hi,
>
> Leonardo Bras <leobras.c@gmail.com> writes:
> > +/**
> > + * rtas_call_reentrant() - Used for reentrant rtas calls
> > + * @token: Token for desired reentrant RTAS call
> > + * @nargs: Number of Input Parameters
> > + * @nret: Number of Output Parameters
> > + * @outputs: Array of outputs
> > + * @...: Inputs for desired RTAS call
> > + *
> > + * According to LoPAR documentation, only "ibm,int-on", "ibm,int-off",
> > + * "ibm,get-xive" and "ibm,set-xive" are currently reentrant.
> > + * Reentrant calls need their own rtas_args buffer, so not using rtas.args, but
> > + * PACA one instead.
> > + *
> > + * Return: -1 on error,
> > + * First output value of RTAS call if (nret > 0),
> > + * 0 otherwise,
> > + */
> > +
> > +int rtas_call_reentrant(int token, int nargs, int nret, int *outputs, ...)
> > +{
> > + va_list list;
> > + struct rtas_args *args;
> > + int i;
> > +
> > + if (!rtas.entry || token == RTAS_UNKNOWN_SERVICE)
> > + return -1;
> > +
> > + /* We use the per-cpu (PACA) rtas args buffer */
> > + args = &local_paca->reentrant_args;
> > +
> > + va_start(list, outputs);
> > + va_rtas_call_unlocked(args, token, nargs, nret, list);
> > + va_end(list);
> > +
> > + if (nret > 1 && outputs)
> > + for (i = 0; i < nret - 1; ++i)
> > + outputs[i] = be32_to_cpu(args->rets[i + 1]);
>
> Doesn't this need to be more careful about preemption and exceptions?
> I.e. the args structure in the paca needs to be protected from
> concurrent use somehow, like any per-cpu data structure.
>
> local_irq_save/restore while accessing local_paca->reentrant_args here
> would be sufficient I think?
Yes, you are right.
I will also add preempt_{dis,en}able, which in most kernels will
compile out, but it will be kind of 'ready' if we ever decide to
support PREEMPT.
Thanks for the feedback!
^ permalink raw reply
* [PATCH v4 0/2] Implement reentrant rtas call
From: Leonardo Bras @ 2020-05-14 23:51 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
Allison Randal, Greg Kroah-Hartman, Thomas Gleixner,
Nicholas Piggin, Leonardo Bras, Nathan Lynch, Gautham R. Shenoy,
Nadav Amit
Cc: linuxppc-dev, linux-kernel
Patch 2 implement rtas_call_reentrant() for reentrant rtas-calls:
"ibm,int-on", "ibm,int-off",ibm,get-xive" and "ibm,set-xive",
according to LoPAPR Version 1.1 (March 24, 2016).
For that, it's necessary that every call uses a different
rtas buffer (rtas_args). Paul Mackerras suggested using the PACA
structure for creating a per-cpu buffer for these calls.
Patch 1 was necessary to make PACA have a 'struct rtas_args' member.
Reentrant rtas calls can be useful to avoid deadlocks in crashing,
where rtas-calls are needed, but some other thread crashed holding
the rtas.lock.
This is a backtrace of a deadlock from a kdump testing environment:
#0 arch_spin_lock
#1 lock_rtas ()
#2 rtas_call (token=8204, nargs=1, nret=1, outputs=0x0)
#3 ics_rtas_mask_real_irq (hw_irq=4100)
#4 machine_kexec_mask_interrupts
#5 default_machine_crash_shutdown
#6 machine_crash_shutdown
#7 __crash_kexec
#8 crash_kexec
#9 oops_end
Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
---
Changes since v3:
- Adds protection from preemption and interruption
Changes since v2:
- Fixed build failure from ppc64e, by including spinlock_types.h on
rtas-types.h
- Improved commit messages
Changes since v1:
- Moved buffer from stack to PACA (as suggested by Paul Mackerras)
- Added missing output bits
- Improve documentation following kernel-doc format (as suggested by
Nathan Lynch)
Leonardo Bras (2):
powerpc/rtas: Move type/struct definitions from rtas.h into
rtas-types.h
powerpc/rtas: Implement reentrant rtas call
arch/powerpc/include/asm/paca.h | 2 +
arch/powerpc/include/asm/rtas-types.h | 126 ++++++++++++++++++++++++++
arch/powerpc/include/asm/rtas.h | 119 +-----------------------
arch/powerpc/kernel/rtas.c | 42 +++++++++
arch/powerpc/sysdev/xics/ics-rtas.c | 22 ++---
5 files changed, 183 insertions(+), 128 deletions(-)
create mode 100644 arch/powerpc/include/asm/rtas-types.h
--
2.25.4
^ permalink raw reply
* [PATCH v4 1/2] powerpc/rtas: Move type/struct definitions from rtas.h into rtas-types.h
From: Leonardo Bras @ 2020-05-14 23:51 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
Greg Kroah-Hartman, Leonardo Bras, Thomas Gleixner,
Allison Randal, Nicholas Piggin, Nathan Lynch, Gautham R. Shenoy,
Nadav Amit
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20200514235138.150722-1-leobras.c@gmail.com>
In order to get any rtas* struct into other headers, including rtas.h
may cause a lot of errors, regarding include dependency needed for
inline functions.
Create rtas-types.h and move there all type/struct definitions
from rtas.h, then include rtas-types.h into rtas.h.
Also, as suggested by checkpath.pl, replace uint8_t for u8, and keep
the same type pattern for the whole file, as they are the same
according to powerpc/boot/types.h.
Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
---
arch/powerpc/include/asm/rtas-types.h | 126 ++++++++++++++++++++++++++
arch/powerpc/include/asm/rtas.h | 118 +-----------------------
2 files changed, 127 insertions(+), 117 deletions(-)
create mode 100644 arch/powerpc/include/asm/rtas-types.h
diff --git a/arch/powerpc/include/asm/rtas-types.h b/arch/powerpc/include/asm/rtas-types.h
new file mode 100644
index 000000000000..87354e28f160
--- /dev/null
+++ b/arch/powerpc/include/asm/rtas-types.h
@@ -0,0 +1,126 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef _POWERPC_RTAS_TYPES_H
+#define _POWERPC_RTAS_TYPES_H
+#ifdef __KERNEL__
+
+#include <linux/spinlock_types.h>
+
+typedef __be32 rtas_arg_t;
+
+struct rtas_args {
+ __be32 token;
+ __be32 nargs;
+ __be32 nret;
+ rtas_arg_t args[16];
+ rtas_arg_t *rets; /* Pointer to return values in args[]. */
+};
+
+struct rtas_t {
+ unsigned long entry; /* physical address pointer */
+ unsigned long base; /* physical address pointer */
+ unsigned long size;
+ arch_spinlock_t lock;
+ struct rtas_args args;
+ struct device_node *dev; /* virtual address pointer */
+};
+
+struct rtas_suspend_me_data {
+ atomic_t working; /* number of cpus accessing this struct */
+ atomic_t done;
+ int token; /* ibm,suspend-me */
+ atomic_t error;
+ struct completion *complete; /* wait on this until working == 0 */
+};
+
+struct rtas_error_log {
+ /* Byte 0 */
+ u8 byte0; /* Architectural version */
+
+ /* Byte 1 */
+ u8 byte1;
+ /* XXXXXXXX
+ * XXX 3: Severity level of error
+ * XX 2: Degree of recovery
+ * X 1: Extended log present?
+ * XX 2: Reserved
+ */
+
+ /* Byte 2 */
+ u8 byte2;
+ /* XXXXXXXX
+ * XXXX 4: Initiator of event
+ * XXXX 4: Target of failed operation
+ */
+ u8 byte3; /* General event or error*/
+ __be32 extended_log_length; /* length in bytes */
+ unsigned char buffer[1]; /* Start of extended log */
+ /* Variable length. */
+};
+
+/* RTAS general extended event log, Version 6. The extended log starts
+ * from "buffer" field of struct rtas_error_log defined above.
+ */
+struct rtas_ext_event_log_v6 {
+ /* Byte 0 */
+ u8 byte0;
+ /* XXXXXXXX
+ * X 1: Log valid
+ * X 1: Unrecoverable error
+ * X 1: Recoverable (correctable or successfully retried)
+ * X 1: Bypassed unrecoverable error (degraded operation)
+ * X 1: Predictive error
+ * X 1: "New" log (always 1 for data returned from RTAS)
+ * X 1: Big Endian
+ * X 1: Reserved
+ */
+
+ /* Byte 1 */
+ u8 byte1; /* reserved */
+
+ /* Byte 2 */
+ u8 byte2;
+ /* XXXXXXXX
+ * X 1: Set to 1 (indicating log is in PowerPC format)
+ * XXX 3: Reserved
+ * XXXX 4: Log format used for bytes 12-2047
+ */
+
+ /* Byte 3 */
+ u8 byte3; /* reserved */
+ /* Byte 4-11 */
+ u8 reserved[8]; /* reserved */
+ /* Byte 12-15 */
+ __be32 company_id; /* Company ID of the company */
+ /* that defines the format for */
+ /* the vendor specific log type */
+ /* Byte 16-end of log */
+ u8 vendor_log[1]; /* Start of vendor specific log */
+ /* Variable length. */
+};
+
+/* Vendor specific Platform Event Log Format, Version 6, section header */
+struct pseries_errorlog {
+ __be16 id; /* 0x00 2-byte ASCII section ID */
+ __be16 length; /* 0x02 Section length in bytes */
+ u8 version; /* 0x04 Section version */
+ u8 subtype; /* 0x05 Section subtype */
+ __be16 creator_component; /* 0x06 Creator component ID */
+ u8 data[]; /* 0x08 Start of section data */
+};
+
+/* RTAS pseries hotplug errorlog section */
+struct pseries_hp_errorlog {
+ u8 resource;
+ u8 action;
+ u8 id_type;
+ u8 reserved;
+ union {
+ __be32 drc_index;
+ __be32 drc_count;
+ struct { __be32 count, index; } ic;
+ char drc_name[1];
+ } _drc_u;
+};
+
+#endif /* __KERNEL__ */
+#endif /* _POWERPC_RTAS_TYPES_H */
diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 3c1887351c71..c35c5350b7e4 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h
@@ -5,6 +5,7 @@
#include <linux/spinlock.h>
#include <asm/page.h>
+#include <asm/rtas-types.h>
#include <linux/time.h>
#include <linux/cpumask.h>
@@ -42,33 +43,6 @@
*
*/
-typedef __be32 rtas_arg_t;
-
-struct rtas_args {
- __be32 token;
- __be32 nargs;
- __be32 nret;
- rtas_arg_t args[16];
- rtas_arg_t *rets; /* Pointer to return values in args[]. */
-};
-
-struct rtas_t {
- unsigned long entry; /* physical address pointer */
- unsigned long base; /* physical address pointer */
- unsigned long size;
- arch_spinlock_t lock;
- struct rtas_args args;
- struct device_node *dev; /* virtual address pointer */
-};
-
-struct rtas_suspend_me_data {
- atomic_t working; /* number of cpus accessing this struct */
- atomic_t done;
- int token; /* ibm,suspend-me */
- atomic_t error;
- struct completion *complete; /* wait on this until working == 0 */
-};
-
/* RTAS event classes */
#define RTAS_INTERNAL_ERROR 0x80000000 /* set bit 0 */
#define RTAS_EPOW_WARNING 0x40000000 /* set bit 1 */
@@ -148,31 +122,6 @@ struct rtas_suspend_me_data {
/* RTAS check-exception vector offset */
#define RTAS_VECTOR_EXTERNAL_INTERRUPT 0x500
-struct rtas_error_log {
- /* Byte 0 */
- uint8_t byte0; /* Architectural version */
-
- /* Byte 1 */
- uint8_t byte1;
- /* XXXXXXXX
- * XXX 3: Severity level of error
- * XX 2: Degree of recovery
- * X 1: Extended log present?
- * XX 2: Reserved
- */
-
- /* Byte 2 */
- uint8_t byte2;
- /* XXXXXXXX
- * XXXX 4: Initiator of event
- * XXXX 4: Target of failed operation
- */
- uint8_t byte3; /* General event or error*/
- __be32 extended_log_length; /* length in bytes */
- unsigned char buffer[1]; /* Start of extended log */
- /* Variable length. */
-};
-
static inline uint8_t rtas_error_severity(const struct rtas_error_log *elog)
{
return (elog->byte1 & 0xE0) >> 5;
@@ -212,47 +161,6 @@ uint32_t rtas_error_extended_log_length(const struct rtas_error_log *elog)
#define RTAS_V6EXT_COMPANY_ID_IBM (('I' << 24) | ('B' << 16) | ('M' << 8))
-/* RTAS general extended event log, Version 6. The extended log starts
- * from "buffer" field of struct rtas_error_log defined above.
- */
-struct rtas_ext_event_log_v6 {
- /* Byte 0 */
- uint8_t byte0;
- /* XXXXXXXX
- * X 1: Log valid
- * X 1: Unrecoverable error
- * X 1: Recoverable (correctable or successfully retried)
- * X 1: Bypassed unrecoverable error (degraded operation)
- * X 1: Predictive error
- * X 1: "New" log (always 1 for data returned from RTAS)
- * X 1: Big Endian
- * X 1: Reserved
- */
-
- /* Byte 1 */
- uint8_t byte1; /* reserved */
-
- /* Byte 2 */
- uint8_t byte2;
- /* XXXXXXXX
- * X 1: Set to 1 (indicating log is in PowerPC format)
- * XXX 3: Reserved
- * XXXX 4: Log format used for bytes 12-2047
- */
-
- /* Byte 3 */
- uint8_t byte3; /* reserved */
- /* Byte 4-11 */
- uint8_t reserved[8]; /* reserved */
- /* Byte 12-15 */
- __be32 company_id; /* Company ID of the company */
- /* that defines the format for */
- /* the vendor specific log type */
- /* Byte 16-end of log */
- uint8_t vendor_log[1]; /* Start of vendor specific log */
- /* Variable length. */
-};
-
static
inline uint8_t rtas_ext_event_log_format(struct rtas_ext_event_log_v6 *ext_log)
{
@@ -287,16 +195,6 @@ inline uint32_t rtas_ext_event_company_id(struct rtas_ext_event_log_v6 *ext_log)
#define PSERIES_ELOG_SECT_ID_HOTPLUG (('H' << 8) | 'P')
#define PSERIES_ELOG_SECT_ID_MCE (('M' << 8) | 'C')
-/* Vendor specific Platform Event Log Format, Version 6, section header */
-struct pseries_errorlog {
- __be16 id; /* 0x00 2-byte ASCII section ID */
- __be16 length; /* 0x02 Section length in bytes */
- uint8_t version; /* 0x04 Section version */
- uint8_t subtype; /* 0x05 Section subtype */
- __be16 creator_component; /* 0x06 Creator component ID */
- uint8_t data[]; /* 0x08 Start of section data */
-};
-
static
inline uint16_t pseries_errorlog_id(struct pseries_errorlog *sect)
{
@@ -309,20 +207,6 @@ inline uint16_t pseries_errorlog_length(struct pseries_errorlog *sect)
return be16_to_cpu(sect->length);
}
-/* RTAS pseries hotplug errorlog section */
-struct pseries_hp_errorlog {
- u8 resource;
- u8 action;
- u8 id_type;
- u8 reserved;
- union {
- __be32 drc_index;
- __be32 drc_count;
- struct { __be32 count, index; } ic;
- char drc_name[1];
- } _drc_u;
-};
-
#define PSERIES_HP_ELOG_RESOURCE_CPU 1
#define PSERIES_HP_ELOG_RESOURCE_MEM 2
#define PSERIES_HP_ELOG_RESOURCE_SLOT 3
--
2.25.4
^ permalink raw reply related
* [PATCH v4 2/2] powerpc/rtas: Implement reentrant rtas call
From: Leonardo Bras @ 2020-05-14 23:51 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
Greg Kroah-Hartman, Leonardo Bras, Thomas Gleixner,
Allison Randal, Nicholas Piggin, Nathan Lynch, Gautham R. Shenoy,
Nadav Amit
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20200514235138.150722-1-leobras.c@gmail.com>
Implement rtas_call_reentrant() for reentrant rtas-calls:
"ibm,int-on", "ibm,int-off",ibm,get-xive" and "ibm,set-xive".
On LoPAPR Version 1.1 (March 24, 2016), from 7.3.10.1 to 7.3.10.4,
items 2 and 3 say:
2 - For the PowerPC External Interrupt option: The * call must be
reentrant to the number of processors on the platform.
3 - For the PowerPC External Interrupt option: The * argument call
buffer for each simultaneous call must be physically unique.
So, these rtas-calls can be called in a lockless way, if using
a different buffer for each cpu doing such rtas call.
For this, it was suggested to add the buffer (struct rtas_args)
in the PACA struct, so each cpu can have it's own buffer.
Reentrant rtas calls are useful to avoid deadlocks in crashing,
where rtas-calls are needed, but some other thread crashed holding
the rtas.lock.
This is a backtrace of a deadlock from a kdump testing environment:
#0 arch_spin_lock
#1 lock_rtas ()
#2 rtas_call (token=8204, nargs=1, nret=1, outputs=0x0)
#3 ics_rtas_mask_real_irq (hw_irq=4100)
#4 machine_kexec_mask_interrupts
#5 default_machine_crash_shutdown
#6 machine_crash_shutdown
#7 __crash_kexec
#8 crash_kexec
#9 oops_end
Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
---
arch/powerpc/include/asm/paca.h | 2 ++
arch/powerpc/include/asm/rtas.h | 1 +
arch/powerpc/kernel/rtas.c | 52 +++++++++++++++++++++++++++++
arch/powerpc/sysdev/xics/ics-rtas.c | 22 ++++++------
4 files changed, 66 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
index e3cc9eb9204d..5a76ba50b40f 100644
--- a/arch/powerpc/include/asm/paca.h
+++ b/arch/powerpc/include/asm/paca.h
@@ -29,6 +29,7 @@
#include <asm/hmi.h>
#include <asm/cpuidle.h>
#include <asm/atomic.h>
+#include <asm/rtas-types.h>
#include <asm-generic/mmiowb_types.h>
@@ -270,6 +271,7 @@ struct paca_struct {
#ifdef CONFIG_MMIOWB
struct mmiowb_state mmiowb_state;
#endif
+ struct rtas_args reentrant_args;
} ____cacheline_aligned;
extern void copy_mm_to_paca(struct mm_struct *mm);
diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index c35c5350b7e4..fa7509c85881 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h
@@ -236,6 +236,7 @@ extern struct rtas_t rtas;
extern int rtas_token(const char *service);
extern int rtas_service_present(const char *service);
extern int rtas_call(int token, int, int, int *, ...);
+int rtas_call_reentrant(int token, int nargs, int nret, int *outputs, ...);
void rtas_call_unlocked(struct rtas_args *args, int token, int nargs,
int nret, ...);
extern void __noreturn rtas_restart(char *cmd);
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index c5fa251b8950..31710b358f44 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -41,6 +41,7 @@
#include <asm/time.h>
#include <asm/mmu.h>
#include <asm/topology.h>
+#include <asm/paca.h>
/* This is here deliberately so it's only used in this file */
void enter_rtas(unsigned long);
@@ -483,6 +484,57 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...)
}
EXPORT_SYMBOL(rtas_call);
+/**
+ * rtas_call_reentrant() - Used for reentrant rtas calls
+ * @token: Token for desired reentrant RTAS call
+ * @nargs: Number of Input Parameters
+ * @nret: Number of Output Parameters
+ * @outputs: Array of outputs
+ * @...: Inputs for desired RTAS call
+ *
+ * According to LoPAR documentation, only "ibm,int-on", "ibm,int-off",
+ * "ibm,get-xive" and "ibm,set-xive" are currently reentrant.
+ * Reentrant calls need their own rtas_args buffer, so not using rtas.args, but
+ * PACA one instead.
+ *
+ * Return: -1 on error,
+ * First output value of RTAS call if (nret > 0),
+ * 0 otherwise,
+ */
+
+int rtas_call_reentrant(int token, int nargs, int nret, int *outputs, ...)
+{
+ va_list list;
+ struct rtas_args *args;
+ unsigned long flags;
+ int i, ret = 0;
+
+ if (!rtas.entry || token == RTAS_UNKNOWN_SERVICE)
+ return -1;
+
+ local_irq_save(flags);
+ preempt_disable();
+
+ /* We use the per-cpu (PACA) rtas args buffer */
+ args = &local_paca->reentrant_args;
+
+ va_start(list, outputs);
+ va_rtas_call_unlocked(args, token, nargs, nret, list);
+ va_end(list);
+
+ if (nret > 1 && outputs)
+ for (i = 0; i < nret - 1; ++i)
+ outputs[i] = be32_to_cpu(args->rets[i + 1]);
+
+ if (nret > 0)
+ ret = be32_to_cpu(args->rets[0]);
+
+ local_irq_restore(flags);
+ preempt_enable();
+
+ return ret;
+}
+
/* For RTAS_BUSY (-2), delay for 1 millisecond. For an extended busy status
* code of 990n, perform the hinted delay of 10^n (last digit) milliseconds.
*/
diff --git a/arch/powerpc/sysdev/xics/ics-rtas.c b/arch/powerpc/sysdev/xics/ics-rtas.c
index 6aabc74688a6..4cf18000f07c 100644
--- a/arch/powerpc/sysdev/xics/ics-rtas.c
+++ b/arch/powerpc/sysdev/xics/ics-rtas.c
@@ -50,8 +50,8 @@ static void ics_rtas_unmask_irq(struct irq_data *d)
server = xics_get_irq_server(d->irq, irq_data_get_affinity_mask(d), 0);
- call_status = rtas_call(ibm_set_xive, 3, 1, NULL, hw_irq, server,
- DEFAULT_PRIORITY);
+ call_status = rtas_call_reentrant(ibm_set_xive, 3, 1, NULL, hw_irq,
+ server, DEFAULT_PRIORITY);
if (call_status != 0) {
printk(KERN_ERR
"%s: ibm_set_xive irq %u server %x returned %d\n",
@@ -60,7 +60,7 @@ static void ics_rtas_unmask_irq(struct irq_data *d)
}
/* Now unmask the interrupt (often a no-op) */
- call_status = rtas_call(ibm_int_on, 1, 1, NULL, hw_irq);
+ call_status = rtas_call_reentrant(ibm_int_on, 1, 1, NULL, hw_irq);
if (call_status != 0) {
printk(KERN_ERR "%s: ibm_int_on irq=%u returned %d\n",
__func__, hw_irq, call_status);
@@ -91,7 +91,7 @@ static void ics_rtas_mask_real_irq(unsigned int hw_irq)
if (hw_irq == XICS_IPI)
return;
- call_status = rtas_call(ibm_int_off, 1, 1, NULL, hw_irq);
+ call_status = rtas_call_reentrant(ibm_int_off, 1, 1, NULL, hw_irq);
if (call_status != 0) {
printk(KERN_ERR "%s: ibm_int_off irq=%u returned %d\n",
__func__, hw_irq, call_status);
@@ -99,8 +99,8 @@ static void ics_rtas_mask_real_irq(unsigned int hw_irq)
}
/* Have to set XIVE to 0xff to be able to remove a slot */
- call_status = rtas_call(ibm_set_xive, 3, 1, NULL, hw_irq,
- xics_default_server, 0xff);
+ call_status = rtas_call_reentrant(ibm_set_xive, 3, 1, NULL, hw_irq,
+ xics_default_server, 0xff);
if (call_status != 0) {
printk(KERN_ERR "%s: ibm_set_xive(0xff) irq=%u returned %d\n",
__func__, hw_irq, call_status);
@@ -131,7 +131,7 @@ static int ics_rtas_set_affinity(struct irq_data *d,
if (hw_irq == XICS_IPI || hw_irq == XICS_IRQ_SPURIOUS)
return -1;
- status = rtas_call(ibm_get_xive, 1, 3, xics_status, hw_irq);
+ status = rtas_call_reentrant(ibm_get_xive, 1, 3, xics_status, hw_irq);
if (status) {
printk(KERN_ERR "%s: ibm,get-xive irq=%u returns %d\n",
@@ -146,8 +146,8 @@ static int ics_rtas_set_affinity(struct irq_data *d,
return -1;
}
- status = rtas_call(ibm_set_xive, 3, 1, NULL,
- hw_irq, irq_server, xics_status[1]);
+ status = rtas_call_reentrant(ibm_set_xive, 3, 1, NULL,
+ hw_irq, irq_server, xics_status[1]);
if (status) {
printk(KERN_ERR "%s: ibm,set-xive irq=%u returns %d\n",
@@ -179,7 +179,7 @@ static int ics_rtas_map(struct ics *ics, unsigned int virq)
return -EINVAL;
/* Check if RTAS knows about this interrupt */
- rc = rtas_call(ibm_get_xive, 1, 3, status, hw_irq);
+ rc = rtas_call_reentrant(ibm_get_xive, 1, 3, status, hw_irq);
if (rc)
return -ENXIO;
@@ -198,7 +198,7 @@ static long ics_rtas_get_server(struct ics *ics, unsigned long vec)
{
int rc, status[2];
- rc = rtas_call(ibm_get_xive, 1, 3, status, vec);
+ rc = rtas_call_reentrant(ibm_get_xive, 1, 3, status, vec);
if (rc)
return -1;
return status[0];
--
2.25.4
^ permalink raw reply related
* Re: [PATCH v3 2/2] powerpc/rtas: Implement reentrant rtas call
From: Leonardo Bras @ 2020-05-15 0:00 UTC (permalink / raw)
To: Nathan Lynch
Cc: Gautham R. Shenoy, Greg Kroah-Hartman, linux-kernel,
Nicholas Piggin, Paul Mackerras, Nadav Amit, Thomas Gleixner,
linuxppc-dev, Allison Randal
In-Reply-To: <fc13b26ff3d2ea2e84049eabda0c60a60d851b40.camel@gmail.com>
On Thu, 2020-05-14 at 20:28 -0300, Leonardo Bras wrote:
> Yes, you are right.
> I will also add preempt_{dis,en}able, which in most kernels will
> compile out, but it will be kind of 'ready' if we ever decide to
> support PREEMPT.
>
> Thanks for the feedback!
v4:
http://patchwork.ozlabs.org/project/linuxppc-dev/patch/20200514235138.150722-3-leobras.c@gmail.com/
^ permalink raw reply
* Re: [PATCH v8 23/30] powerpc: Add prefixed instructions to instruction data type
From: Michael Ellerman @ 2020-05-15 1:33 UTC (permalink / raw)
To: Christophe Leroy, Jordan Niethe, linuxppc-dev
Cc: christophe.leroy, alistair, npiggin, bala24, naveen.n.rao, dja
In-Reply-To: <56ca6bcb-c719-a049-63b0-aae73023bde5@csgroup.eu>
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 06/05/2020 à 05:40, Jordan Niethe a écrit :
>> For powerpc64, redefine the ppc_inst type so both word and prefixed
>> instructions can be represented. On powerpc32 the type will remain the
>> same. Update places which had assumed instructions to be 4 bytes long.
...
>> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
>> index c0a35e4586a5..217897927926 100644
>> --- a/arch/powerpc/include/asm/uaccess.h
>> +++ b/arch/powerpc/include/asm/uaccess.h
>> @@ -105,11 +105,49 @@ static inline int __access_ok(unsigned long addr, unsigned long size,
>> #define __put_user_inatomic(x, ptr) \
>> __put_user_nosleep((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
>>
>> +#ifdef __powerpc64__
>
> Replace by CONFIG_PPC64
>
>> +#define __get_user_instr(x, ptr) \
>> +({ \
>> + long __gui_ret = 0; \
>> + unsigned long __gui_ptr = (unsigned long)ptr; \
>> + struct ppc_inst __gui_inst; \
>> + unsigned int prefix, suffix; \
>> + __gui_ret = __get_user(prefix, (unsigned int __user *)__gui_ptr); \
>
> __get_user() can be costly especially with KUAP. I think you should
> perform a 64 bits read and fallback on a 32 bits read only if the 64
> bits read failed.
I worry that might break something.
It _should_ be safe, but I'm paranoid.
If we think the KUAP overhead is a problem then I think we'd be better
off pulling the KUAP disable/enable into this macro.
>> diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c
>> index 2bd2b752de4f..a8238eff3a31 100644
>> --- a/arch/powerpc/lib/feature-fixups.c
>> +++ b/arch/powerpc/lib/feature-fixups.c
>> @@ -84,12 +84,13 @@ static int patch_feature_section(unsigned long value, struct fixup_entry *fcur)
>> src = alt_start;
>> dest = start;
>>
>> - for (; src < alt_end; src++, dest++) {
>> + for (; src < alt_end; src = (void *)src + ppc_inst_len(ppc_inst_read(src)),
>> + (dest = (void *)dest + ppc_inst_len(ppc_inst_read(dest)))) {
>
> Can we do this outside the for() for readability ?
I have an idea for cleaning these up, will post it as a follow-up to the series.
>> diff --git a/arch/powerpc/lib/inst.c b/arch/powerpc/lib/inst.c
>> index 08dedd927268..eb6f9ee28ac6 100644
>> --- a/arch/powerpc/lib/inst.c
>> +++ b/arch/powerpc/lib/inst.c
>> @@ -3,9 +3,49 @@
>> * Copyright 2020, IBM Corporation.
>> */
>>
>> +#include <asm/ppc-opcode.h>
>> #include <linux/uaccess.h>
>> #include <asm/inst.h>
>>
>> +#ifdef __powerpc64__
>> +int probe_user_read_inst(struct ppc_inst *inst,
>> + struct ppc_inst *nip)
>> +{
>> + unsigned int val, suffix;
>> + int err;
>> +
>> + err = probe_user_read(&val, nip, sizeof(val));
>
> A user read is costly with KUAP. Can we do a 64 bits read and perform a
> 32 bits read only when 64 bits read fails ?
Same comment as above.
cheers
^ permalink raw reply
* Re: [PATCH v2 7/9] powerpc/ps3: Add check for otheros image size
From: Michael Ellerman @ 2020-05-15 2:02 UTC (permalink / raw)
To: Geoff Levand
Cc: linuxppc-dev, Geert Uytterhoeven, Markus Elfring,
Emmanuel Nicolet
In-Reply-To: <4e8defeb49d62dd9d435e5ea3ddc5668e56fa496.1589049250.git.geoff@infradead.org>
Hi Geoff,
Geoff Levand <geoff@infradead.org> writes:
> The ps3's otheros flash loader has a size limit of 16 MiB for the
> uncompressed image. If that limit will be reached output the
> flash image file as 'otheros-too-big.bld'.
>
> Signed-off-by: Geoff Levand <geoff@infradead.org>
> ---
> arch/powerpc/boot/wrapper | 17 +++++++++++++++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/boot/wrapper b/arch/powerpc/boot/wrapper
> index 35ace40d9fc2..ab1e3ddc79f3 100755
> --- a/arch/powerpc/boot/wrapper
> +++ b/arch/powerpc/boot/wrapper
> @@ -571,7 +571,20 @@ ps3)
> count=$overlay_size bs=1
>
> odir="$(dirname "$ofile.bin")"
> - rm -f "$odir/otheros.bld"
> - gzip -n --force -9 --stdout "$ofile.bin" > "$odir/otheros.bld"
> +
> + # The ps3's flash loader has a size limit of 16 MiB for the uncompressed
> + # image. If a compressed image that exceeded this limit is written to
> + # flash the loader will decompress that image until the 16 MiB limit is
> + # reached, then enter the system reset vector of the partially decompressed
> + # image. No warning is issued.
> + rm -f "$odir"/{otheros,otheros-too-big}.bld
> + size=$(${CROSS}nm --no-sort --radix=d "$ofile" | egrep ' _end$' | cut -d' ' -f1)
> + bld="otheros.bld"
> + if [ $size -gt $((0x1000000)) ]; then
> + bld="otheros-too-big.bld"
> + echo " INFO: Uncompressed kernel is too large to program into PS3 flash memory;" \
This now appears on all my ppc64_defconfig builds, which I don't really
like.
That does highlight the fact that ppc64_defconfig including
CONFIG_PPC_PS3 is not really helpful for people actually wanting to run
the kernel on a PS3.
So I wonder if we should drop CONFIG_PPC_PS3 from ppc64_defconfig, in
which case I'd be happy to keep the INFO message because it should only
appear on ps3 specific builds.
The other option would be to drop the message, or only print it when
we're doing a verbose build.
Thoughts?
cheers
^ permalink raw reply
* Re: [PATCH v8 13/30] powerpc: Add a probe_user_read_inst() function
From: Jordan Niethe @ 2020-05-15 3:46 UTC (permalink / raw)
To: Christophe Leroy
Cc: Christophe Leroy, Alistair Popple, Nicholas Piggin, Balamuruhan S,
naveen.n.rao, linuxppc-dev, Daniel Axtens
In-Reply-To: <e4bd592e-2e35-9c73-854e-1380222f34ef@csgroup.eu>
On Thu, May 14, 2020 at 3:46 PM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
>
>
> Le 06/05/2020 à 05:40, Jordan Niethe a écrit :
> > Introduce a probe_user_read_inst() function to use in cases where
> > probe_user_read() is used for getting an instruction. This will be more
> > useful for prefixed instructions.
> >
> > Reviewed-by: Alistair Popple <alistair@popple.id.au>
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > v6: - New to series
> > ---
> > arch/powerpc/include/asm/inst.h | 3 +++
> > arch/powerpc/lib/Makefile | 2 +-
> > arch/powerpc/lib/inst.c | 18 ++++++++++++++++++
> > arch/powerpc/mm/fault.c | 2 +-
> > 4 files changed, 23 insertions(+), 2 deletions(-)
> > create mode 100644 arch/powerpc/lib/inst.c
> >
> > diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
> > index 552e953bf04f..3e9a58420151 100644
> > --- a/arch/powerpc/include/asm/inst.h
> > +++ b/arch/powerpc/include/asm/inst.h
> > @@ -37,4 +37,7 @@ static inline bool ppc_inst_equal(struct ppc_inst x, struct ppc_inst y)
> > return ppc_inst_val(x) == ppc_inst_val(y);
> > }
> >
> > +int probe_user_read_inst(struct ppc_inst *inst,
> > + struct ppc_inst *nip);
> > +
> > #endif /* _ASM_INST_H */
> > diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
> > index b8de3be10eb4..546591848219 100644
> > --- a/arch/powerpc/lib/Makefile
> > +++ b/arch/powerpc/lib/Makefile
> > @@ -16,7 +16,7 @@ CFLAGS_code-patching.o += -DDISABLE_BRANCH_PROFILING
> > CFLAGS_feature-fixups.o += -DDISABLE_BRANCH_PROFILING
> > endif
> >
> > -obj-y += alloc.o code-patching.o feature-fixups.o pmem.o
> > +obj-y += alloc.o code-patching.o feature-fixups.o pmem.o inst.o
> >
> > ifndef CONFIG_KASAN
> > obj-y += string.o memcmp_$(BITS).o
> > diff --git a/arch/powerpc/lib/inst.c b/arch/powerpc/lib/inst.c
> > new file mode 100644
> > index 000000000000..eaf786afad2b
> > --- /dev/null
> > +++ b/arch/powerpc/lib/inst.c
> > @@ -0,0 +1,18 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright 2020, IBM Corporation.
> > + */
> > +
> > +#include <linux/uaccess.h>
> > +#include <asm/inst.h>
> > +
> > +int probe_user_read_inst(struct ppc_inst *inst,
> > + struct ppc_inst *nip)
> > +{
> > + unsigned int val;
> > + int err;
> > +
> > + err = probe_user_read(&val, nip, sizeof(val));
> > + *inst = ppc_inst(val);
> > + return err;
> > +}
> > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> > index 4a50f125ec18..f3a943eae305 100644
> > --- a/arch/powerpc/mm/fault.c
> > +++ b/arch/powerpc/mm/fault.c
> > @@ -281,7 +281,7 @@ static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address,
> > access_ok(nip, sizeof(*nip))) {
> > struct ppc_inst inst;
> >
> > - if (!probe_user_read(&inst, nip, sizeof(inst)))
> > + if (!probe_user_read_inst(&inst, (struct ppc_inst __user *)nip))
>
> Shouldn't 'nip' become de 'struct ppc_inst __user *' instead of casting ?
>
> > return !store_updates_sp(inst);
> > *must_retry = true;
> > }
> >
Yeah it would make more sense to do it like this.
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -256,7 +256,7 @@ static bool bad_stack_expansion(struct pt_regs
*regs, unsigned long address,
* expand to 1MB without further checks.
*/
if (address + 0x100000 < vma->vm_end) {
- unsigned int __user *nip = (unsigned int __user *)regs->nip;
+ struct ppc_inst __user *nip = (struct ppc_inst __user *)regs->nip;
/* get user regs even if this fault is in kernel mode */
struct pt_regs *uregs = current->thread.regs;
if (uregs == NULL)
@@ -281,7 +281,7 @@ static bool bad_stack_expansion(struct pt_regs
*regs, unsigned long address,
access_ok(nip, sizeof(*nip))) {
struct ppc_inst inst;
- if (!probe_user_read_inst(&inst, (struct ppc_inst __user *)nip))
+ if (!probe_user_read_inst(&inst, nip))
return !store_updates_sp(inst);
*must_retry = true;
}
--
2.17.1
>
> Christophe
^ permalink raw reply
* Re: [PATCH v4 2/2] powerpc/rtas: Implement reentrant rtas call
From: Nicholas Piggin @ 2020-05-15 7:30 UTC (permalink / raw)
To: Allison Randal, Benjamin Herrenschmidt, Gautham R. Shenoy,
Greg Kroah-Hartman, Leonardo Bras, Michael Ellerman, Nadav Amit,
Nathan Lynch, Paul Mackerras, Thomas Gleixner
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20200514235138.150722-3-leobras.c@gmail.com>
Excerpts from Leonardo Bras's message of May 15, 2020 9:51 am:
> Implement rtas_call_reentrant() for reentrant rtas-calls:
> "ibm,int-on", "ibm,int-off",ibm,get-xive" and "ibm,set-xive".
>
> On LoPAPR Version 1.1 (March 24, 2016), from 7.3.10.1 to 7.3.10.4,
> items 2 and 3 say:
>
> 2 - For the PowerPC External Interrupt option: The * call must be
> reentrant to the number of processors on the platform.
> 3 - For the PowerPC External Interrupt option: The * argument call
> buffer for each simultaneous call must be physically unique.
>
> So, these rtas-calls can be called in a lockless way, if using
> a different buffer for each cpu doing such rtas call.
What about rtas_call_unlocked? Do the callers need to take the rtas
lock?
Machine checks must call ibm,nmi-interlock too, which we really don't
want to take a lock for either. Hopefully that's in a class of its own
and we can essentially ignore with respect to other rtas calls.
The spec is pretty vague too :(
"The ibm,get-xive call must be reentrant to the number of processors on
the platform."
This suggests ibm,get-xive can be called concurrently by multiple
processors. It doesn't say anything about being re-entrant against any
of the other re-entrant calls. Maybe that could be reasonably assumed,
but I don't know if it's reasonable to assume it can be called
concurrently with a *non-reentrant* call, is it?
> For this, it was suggested to add the buffer (struct rtas_args)
> in the PACA struct, so each cpu can have it's own buffer.
You can't do this, paca is not limited to RTAS_INSTANTIATE_MAX.
Which is good, because I didn't want you to add another 88 bytes to the
paca :) Can you make it a pointer and allocate it separately? Check
the slb_shadow allocation, you could use a similar pattern.
The other option would be to have just one more rtas args, and have the
crashing CPU always that. That would skirt the re-entrancy issue -- the
concurrency is only ever a last resort. Would be a bit tricker though.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH] tty: hvc: Fix data abort due to race in hvc_open
From: Greg KH @ 2020-05-15 7:30 UTC (permalink / raw)
To: rananta; +Cc: linuxppc-dev, andrew, Jiri Slaby, linux-kernel
In-Reply-To: <0ab0b49f19b824ac1c4db4c4937ed388@codeaurora.org>
On Thu, May 14, 2020 at 04:22:10PM -0700, rananta@codeaurora.org wrote:
> On 2020-05-13 00:04, Greg KH wrote:
> > On Tue, May 12, 2020 at 02:39:50PM -0700, rananta@codeaurora.org wrote:
> > > On 2020-05-12 01:25, Greg KH wrote:
> > > > On Tue, May 12, 2020 at 09:22:15AM +0200, Jiri Slaby wrote:
> > > > > On 11. 05. 20, 9:39, Greg KH wrote:
> > > > > > On Mon, May 11, 2020 at 12:23:58AM -0700, rananta@codeaurora.org wrote:
> > > > > >> On 2020-05-09 23:48, Greg KH wrote:
> > > > > >>> On Sat, May 09, 2020 at 06:30:56PM -0700, rananta@codeaurora.org wrote:
> > > > > >>>> On 2020-05-06 02:48, Greg KH wrote:
> > > > > >>>>> On Mon, Apr 27, 2020 at 08:26:01PM -0700, Raghavendra Rao Ananta wrote:
> > > > > >>>>>> Potentially, hvc_open() can be called in parallel when two tasks calls
> > > > > >>>>>> open() on /dev/hvcX. In such a scenario, if the
> > > > > >>>>>> hp->ops->notifier_add()
> > > > > >>>>>> callback in the function fails, where it sets the tty->driver_data to
> > > > > >>>>>> NULL, the parallel hvc_open() can see this NULL and cause a memory
> > > > > >>>>>> abort.
> > > > > >>>>>> Hence, serialize hvc_open and check if tty->private_data is NULL
> > > > > >>>>>> before
> > > > > >>>>>> proceeding ahead.
> > > > > >>>>>>
> > > > > >>>>>> The issue can be easily reproduced by launching two tasks
> > > > > >>>>>> simultaneously
> > > > > >>>>>> that does nothing but open() and close() on /dev/hvcX.
> > > > > >>>>>> For example:
> > > > > >>>>>> $ ./simple_open_close /dev/hvc0 & ./simple_open_close /dev/hvc0 &
> > > > > >>>>>>
> > > > > >>>>>> Signed-off-by: Raghavendra Rao Ananta <rananta@codeaurora.org>
> > > > > >>>>>> ---
> > > > > >>>>>> drivers/tty/hvc/hvc_console.c | 16 ++++++++++++++--
> > > > > >>>>>> 1 file changed, 14 insertions(+), 2 deletions(-)
> > > > > >>>>>>
> > > > > >>>>>> diff --git a/drivers/tty/hvc/hvc_console.c
> > > > > >>>>>> b/drivers/tty/hvc/hvc_console.c
> > > > > >>>>>> index 436cc51c92c3..ebe26fe5ac09 100644
> > > > > >>>>>> --- a/drivers/tty/hvc/hvc_console.c
> > > > > >>>>>> +++ b/drivers/tty/hvc/hvc_console.c
> > > > > >>>>>> @@ -75,6 +75,8 @@ static LIST_HEAD(hvc_structs);
> > > > > >>>>>> */
> > > > > >>>>>> static DEFINE_MUTEX(hvc_structs_mutex);
> > > > > >>>>>>
> > > > > >>>>>> +/* Mutex to serialize hvc_open */
> > > > > >>>>>> +static DEFINE_MUTEX(hvc_open_mutex);
> > > > > >>>>>> /*
> > > > > >>>>>> * This value is used to assign a tty->index value to a hvc_struct
> > > > > >>>>>> based
> > > > > >>>>>> * upon order of exposure via hvc_probe(), when we can not match it
> > > > > >>>>>> to
> > > > > >>>>>> @@ -346,16 +348,24 @@ static int hvc_install(struct tty_driver
> > > > > >>>>>> *driver, struct tty_struct *tty)
> > > > > >>>>>> */
> > > > > >>>>>> static int hvc_open(struct tty_struct *tty, struct file * filp)
> > > > > >>>>>> {
> > > > > >>>>>> - struct hvc_struct *hp = tty->driver_data;
> > > > > >>>>>> + struct hvc_struct *hp;
> > > > > >>>>>> unsigned long flags;
> > > > > >>>>>> int rc = 0;
> > > > > >>>>>>
> > > > > >>>>>> + mutex_lock(&hvc_open_mutex);
> > > > > >>>>>> +
> > > > > >>>>>> + hp = tty->driver_data;
> > > > > >>>>>> + if (!hp) {
> > > > > >>>>>> + rc = -EIO;
> > > > > >>>>>> + goto out;
> > > > > >>>>>> + }
> > > > > >>>>>> +
> > > > > >>>>>> spin_lock_irqsave(&hp->port.lock, flags);
> > > > > >>>>>> /* Check and then increment for fast path open. */
> > > > > >>>>>> if (hp->port.count++ > 0) {
> > > > > >>>>>> spin_unlock_irqrestore(&hp->port.lock, flags);
> > > > > >>>>>> hvc_kick();
> > > > > >>>>>> - return 0;
> > > > > >>>>>> + goto out;
> > > > > >>>>>> } /* else count == 0 */
> > > > > >>>>>> spin_unlock_irqrestore(&hp->port.lock, flags);
> > > > > >>>>>
> > > > > >>>>> Wait, why isn't this driver just calling tty_port_open() instead of
> > > > > >>>>> trying to open-code all of this?
> > > > > >>>>>
> > > > > >>>>> Keeping a single mutext for open will not protect it from close, it will
> > > > > >>>>> just slow things down a bit. There should already be a tty lock held by
> > > > > >>>>> the tty core for open() to keep it from racing things, right?
> > > > > >>>> The tty lock should have been held, but not likely across
> > > > > >>>> ->install() and
> > > > > >>>> ->open() callbacks, thus resulting in a race between hvc_install() and
> > > > > >>>> hvc_open(),
> > > > > >>>
> > > > > >>> How? The tty lock is held in install, and should not conflict with
> > > > > >>> open(), otherwise, we would be seeing this happen in all tty drivers,
> > > > > >>> right?
> > > > > >>>
> > > > > >> Well, I was expecting the same, but IIRC, I see that the open() was being
> > > > > >> called in parallel for the same device node.
> > > > > >
> > > > > > So open and install are happening at the same time? And the tty_lock()
> > > > > > does not protect the needed fields from being protected properly? If
> > > > > > not, what fields are being touched without the lock?
> > > > > >
> > > > > >> Is it expected that the tty core would allow only one thread to
> > > > > >> access the dev-node, while blocking the other, or is it the client
> > > > > >> driver's responsibility to handle the exclusiveness?
> > > > > >
> > > > > > The tty core should handle this correctly, for things that can mess
> > > > > > stuff up (like install and open at the same time). A driver should not
> > > > > > have to worry about that.
> > > > > >
> > > > > >>>> where hvc_install() sets a data and the hvc_open() clears it.
> > > > > >>>> hvc_open()
> > > > > >>>> doesn't
> > > > > >>>> check if the data was set to NULL and proceeds.
> > > > > >>>
> > > > > >>> What data is being set that hvc_open is checking?
> > > > > >> hvc_install sets tty->private_data to hp, while hvc_open sets it to NULL (in
> > > > > >> one of the paths).
> > > > > >
> > > > > > I see no use of private_data in drivers/tty/hvc/ so what exactly are you
> > > > > > referring to?
> > > > >
> > > > > He likely means tty->driver_data. And there exactly lays the issue.
> > > > >
> > > > > commit bdb498c20040616e94b05c31a0ceb3e134b7e829
> > > > > Author: Jiri Slaby <jslaby@suse.cz>
> > > > > Date: Tue Aug 7 21:48:04 2012 +0200
> > > > >
> > > > > TTY: hvc_console, add tty install
> > > > >
> > > > > added hvc_install but did not move 'tty->driver_data = NULL;' from
> > > > > hvc_open's fail path to hvc_cleanup.
> > > > >
> > > > > IOW hvc_open now NULLs tty->driver_data even for another task which
> > > > > opened the tty earlier. The same holds for
> > > > > "tty_port_tty_set(&hp->port,
> > > > > NULL);" there. And actually "tty_port_put(&hp->port);" is also
> > > > > incorrect
> > > > > for the 2nd task opening the tty.
> > > > >
> > > > > So, a mutex with tty->driver_data check in open is not definitely the
> > > > > way to go. This mess needs to be sorted out properly. Sure, a good
> > > > > start
> > > > > would be a conversion to tty_port_open. Right after dropping "tty:
> > > > > hvc:
> > > > > Fix data abort due to race in hvc_open" from tty/tty-next :).
> > > >
> > > > I've now reverted this commit so we can start from a "clean" place.
> > > >
> > > > > What I *don't* understand is why hp->ops->notifier_add fails, given
> > > > > the
> > > > > open does not allow multiple opens anyway?
> > > >
> > > > I don't understand that either. Raghavendra, can you show a real trace
> > > > for this issue that shows this?
> > > >
> > > Let me know if this helps:
> > >
> > > [ 265.332900] Unable to handle kernel NULL pointer dereference at
> > > virtual
> > > address 00000000000000a8
> > > [ 265.332920] Mem abort info:
> > > [ 265.332934] ESR = 0x96000006
> > > [ 265.332950] EC = 0x25: DABT (current EL), IL = 32 bits
> > > [ 265.332963] SET = 0, FnV = 0
> > > [ 265.332975] EA = 0, S1PTW = 0
> > > [ 265.332985] Data abort info:
> > > [ 265.332997] ISV = 0, ISS = 0x00000006
> > > [ 265.333008] CM = 0, WnR = 0
> > > [ 265.333025] user pgtable: 4k pages, 39-bit VAs,
> > > pgdp=00000001620f3000
> > > [ 265.333038] [00000000000000a8] pgd=00000001620f2003,
> > > pud=00000001620f2003, pmd=0000000000000000
> > > [ 265.333071] Internal error: Oops: 96000006 [#1] PREEMPT SMP
> > > [ 265.333424] CPU: 1 PID: 5653 Comm: stress-ng-dev Tainted: G S
> > > W O
> > > 5.4.12-g04866e0 #1
> > > [ 265.333458] pstate: 80400085 (Nzcv daIf +PAN -UAO)
> > > [ 265.333499] pc : _raw_spin_lock_irqsave+0x40/0x7c
> > > [ 265.333517] lr : _raw_spin_lock_irqsave+0x38/0x7c
> > > [ 265.333530] sp : ffffffc02436ba40
> > > [ 265.333542] x29: ffffffc02436ba40 x28: 0000000000020800
> > > [ 265.333562] x27: ffffffdfb4046490 x26: ffffff8101b83400
> > > [ 265.333580] x25: ffffff80e163ad00 x24: ffffffdfb45c7798
> > > [ 265.333598] x23: ffffff8101b83668 x22: ffffffdfb4974000
> > > [ 265.333617] x21: 0000000000000001 x20: 00000000000000a8
> > > [ 265.333634] x19: 0000000000000000 x18: ffffff80e0b0d460
> > > [ 265.333652] x17: 0000000000000000 x16: 0000000001000000
> > > [ 265.333670] x15: 0000000001000000 x14: 00000000f8000000
> > > [ 265.333688] x13: 0000000000000000 x12: 0000000000000001
> > > [ 265.333706] x11: 17f5f16765f64600 x10: 17f5f16765f64600
> > > [ 265.333724] x9 : ffffffdfb3444244 x8 : 0000000000000000
> > > [ 265.333741] x7 : 0000000000000000 x6 : 0000000000000000
> > > [ 265.333759] x5 : 0000000000000000 x4 : 0000000000000002
> > > [ 265.333776] x3 : ffffffc02436b9c0 x2 : ffffffdfb40456e0
> > > [ 265.333794] x1 : ffffffc02436b9c0 x0 : ffffffdfb3444244
> > > [ 265.333812] Call trace:
> > > [ 265.333831] _raw_spin_lock_irqsave+0x40/0x7c
> > > [ 265.333859] 28$61deaf328f140fd7df47c115ec866fa5+0x28/0x174
> > > [ 265.333882] tty_open$86bd494905ebe22944bf63b711173de3+0x3d0/0x584
> > > [ 265.333921]
> > > chrdev_open$4083aaa799bca8e0e1e0c8dc1947aa96+0x1c4/0x248
> > > [ 265.333940] do_dentry_open+0x258/0x3b0
> > > [ 265.333956] vfs_open+0x2c/0x38
> > > [ 265.333975] path_openat+0x898/0xedc
> > > [ 265.333991] do_filp_open+0x78/0x124
> > > [ 265.334006] do_sys_open+0x13c/0x298
> > > [ 265.334022] __arm64_sys_openat+0x28/0x34
> > > [ 265.334044] el0_svc_common+0xb8/0x1b4
> > > [ 265.334059] el0_svc_handler+0x6c/0x88
> > > [ 265.334079] el0_svc+0x8/0xc
> > > [ 265.334110] Code: 52800035 97b9fec7 aa1f03e8 f9800291 (885ffe81)
> > > [ 265.334130] ---[ end trace ac90e3099a98e99f ]---
> > > [ 265.334146] Kernel panic - not syncing: Fatal exception
> >
> > Hm, do you have a strace showing the close happening at the same time?
> > What about install()?
>
> Yes, I do see the close happening, at which point the issue is not seen.
> It's only seen if the second task came in before this close was called. For
> this task, as the file was already opened, it doesn't go through
> hvc_install.
>
> (I figured adding some logs in the drivers would be helpful than straces to
> also include hvc_install)
>
> These are the traces you get when the issue happens:
> [ 154.212291] hvc_install called for pid: 666
> [ 154.216705] hvc_open called for pid: 666
> [ 154.233657] hvc_open: request_irq failed with rc -22.
> [ 154.238934] hvc_open called for pid: 678
> [ 154.243012] Unable to handle kernel NULL pointer dereference at virtual
> address 00000000000000c4
> # hvc_install isn't called for pid: 678 as the file wasn't closed yet.
>
> And these are the traces we get when things are normal:
> [ 76.318861] hvc_install called for pid: 537
> [ 76.323240] hvc_open called for pid: 537
> [ 76.340177] hvc_open: request_irq failed with rc -22.
> [ 76.345384] hvc_close called for pid: 537
> [ 76.349555] hvc_install called for pid: 538
> [ 76.353921] hvc_open called for pid: 538
> # hvc_open for the second task (pid: 538) seems to be fine here as the file
> was closed prior to the second task tried to open the file.
>
> >
> > And what line in hvc_open() does that offset correspond to?
> It's the point where it tries to acquire the spinlock for the first time:
> spin_lock_irqsave(&hp->port.lock, flags);
Ah, is this a console? Maybe this is the same issue that other console
drivers have been having recently, look at:
https://lore.kernel.org/r/20200428184050.6501-1-john.stultz@linaro.org
and
https://lore.kernel.org/r/1589019852-21505-2-git-send-email-sagar.kadam@sifive.com
Is that what you need here too?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v8 08/30] powerpc: Use a function for getting the instruction op code
From: Jordan Niethe @ 2020-05-15 7:48 UTC (permalink / raw)
To: linuxppc-dev
Cc: Christophe Leroy, Alistair Popple, Nicholas Piggin, Balamuruhan S,
naveen.n.rao, Daniel Axtens
In-Reply-To: <20200506034050.24806-9-jniethe5@gmail.com>
mpe, as suggested by Christophe could you please add this.
diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
--- a/arch/powerpc/include/asm/inst.h
+++ b/arch/powerpc/include/asm/inst.h
@@ -2,6 +2,8 @@
#ifndef _ASM_INST_H
#define _ASM_INST_H
+#include <asm/disassemble.h>
+
/*
* Instruction data type for POWER
*/
@@ -15,7 +17,7 @@ static inline u32 ppc_inst_val(u32 x)
static inline int ppc_inst_primary_opcode(u32 x)
{
- return ppc_inst_val(x) >> 26;
+ return get_op(ppc_inst_val(x));
}
#endif /* _ASM_INST_H */
--
2.17.1
^ permalink raw reply
* Re: [PATCH v8 23/30] powerpc: Add prefixed instructions to instruction data type
From: Jordan Niethe @ 2020-05-15 7:52 UTC (permalink / raw)
To: Michael Ellerman
Cc: Christophe Leroy, Alistair Popple, Nicholas Piggin, Balamuruhan S,
naveen.n.rao, linuxppc-dev, Daniel Axtens
In-Reply-To: <871rnmasnn.fsf@mpe.ellerman.id.au>
Hey mpe, fixes for the issues highlighted by Christophe, except KUAP
as discussed. Will make the optprobe change as a preceding patch.
diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
--- a/arch/powerpc/include/asm/inst.h
+++ b/arch/powerpc/include/asm/inst.h
@@ -11,9 +11,9 @@
struct ppc_inst {
u32 val;
-#ifdef __powerpc64__
+#ifdef CONFIG_PPC64
u32 suffix;
-#endif /* __powerpc64__ */
+#endif /* CONFIG_PPC64 */
} __packed;
static inline u32 ppc_inst_val(struct ppc_inst x)
@@ -26,7 +26,7 @@ static inline int ppc_inst_primary_opcode(struct ppc_inst x)
return get_op(ppc_inst_val(x));
}
-#ifdef __powerpc64__
+#ifdef CONFIG_PPC64
#define ppc_inst(x) ((struct ppc_inst){ .val = (x), .suffix = 0xff })
#define ppc_inst_prefix(x, y) ((struct ppc_inst){ .val = (x), .suffix = (y) })
@@ -52,7 +52,7 @@ static inline struct ppc_inst ppc_inst_read(const
struct ppc_inst *ptr)
u32 val, suffix;
val = *(u32 *)ptr;
- if ((val >> 26) == 1) {
+ if ((get_op(val)) == OP_PREFIX) {
suffix = *((u32 *)ptr + 1);
return ppc_inst_prefix(val, suffix);
} else {
@@ -94,7 +94,7 @@ static inline bool ppc_inst_equal(struct ppc_inst x,
struct ppc_inst y)
return ppc_inst_val(x) == ppc_inst_val(y);
}
-#endif /* __powerpc64__ */
+#endif /* CONFIG_PPC64 */
static inline int ppc_inst_len(struct ppc_inst x)
{
diff --git a/arch/powerpc/include/asm/uaccess.h
b/arch/powerpc/include/asm/uaccess.h
index e9027b3c641a..ac36a82321d4 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -105,7 +105,7 @@ static inline int __access_ok(unsigned long addr,
unsigned long size,
#define __put_user_inatomic(x, ptr) \
__put_user_nosleep((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
-#ifdef __powerpc64__
+#ifdef CONFIG_PPC64
#define __get_user_instr(x, ptr) \
({ \
long __gui_ret = 0; \
@@ -113,7 +113,7 @@ static inline int __access_ok(unsigned long addr,
unsigned long size,
struct ppc_inst __gui_inst; \
unsigned int prefix, suffix; \
__gui_ret = __get_user(prefix, (unsigned int __user *)__gui_ptr); \
- if (!__gui_ret && (prefix >> 26) == OP_PREFIX) { \
+ if (!__gui_ret && (get_op(prefix)) == OP_PREFIX) { \
__gui_ret = __get_user(suffix, \
(unsigned int __user *)__gui_ptr + 1); \
__gui_inst = ppc_inst_prefix(prefix, suffix); \
@@ -131,7 +131,7 @@ static inline int __access_ok(unsigned long addr,
unsigned long size,
struct ppc_inst __gui_inst; \
unsigned int prefix, suffix; \
__gui_ret = __get_user_inatomic(prefix, (unsigned int __user
*)__gui_ptr); \
- if (!__gui_ret && (prefix >> 26) == OP_PREFIX) { \
+ if (!__gui_ret && (get_op(prefix)) == OP_PREFIX) { \
__gui_ret = __get_user_inatomic(suffix, \
(unsigned int __user *)__gui_ptr + 1); \
__gui_inst = ppc_inst_prefix(prefix, suffix); \
diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index a8e66603d12b..3ac105e7faae 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -283,10 +283,8 @@ int arch_prepare_optimized_kprobe(struct
optimized_kprobe *op, struct kprobe *p)
* 3. load instruction to be emulated into relevant register, and
*/
temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);
- patch_imm64_load_insns(ppc_inst_val(temp) |
- ((u64)ppc_inst_suffix(temp) << 32),
- 4,
- buff + TMPL_INSN_IDX);
+ patch_imm64_load_insns(ppc_inst_val(temp) |
((u64)ppc_inst_suffix(temp) << 32),
+ 4, buff + TMPL_INSN_IDX);
/*
* 4. branch back from trampoline
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index 58b67b62d5d3..bfd4e1dae0fb 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -26,8 +26,6 @@ static int __patch_instruction(struct ppc_inst
*exec_addr, struct ppc_inst instr
if (!ppc_inst_prefixed(instr)) {
__put_user_asm(ppc_inst_val(instr), patch_addr, err, "stw");
- if (err)
- return err;
} else {
#ifdef CONFIG_CPU_LITTLE_ENDIAN
__put_user_asm((u64)ppc_inst_suffix(instr) << 32 |
@@ -36,12 +34,13 @@ static int __patch_instruction(struct ppc_inst
*exec_addr, struct ppc_inst instr
__put_user_asm((u64)ppc_inst_val(instr) << 32 |
ppc_inst_suffix(instr), patch_addr, err, "std");
#endif /* CONFIG_CPU_LITTLE_ENDIAN */
- if (err)
- return err;
}
+ if (err)
+ return err;
asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (patch_addr),
"r" (exec_addr));
+
return 0;
}
diff --git a/arch/powerpc/lib/inst.c b/arch/powerpc/lib/inst.c
index e5e589994097..3c3851ffdb36 100644
--- a/arch/powerpc/lib/inst.c
+++ b/arch/powerpc/lib/inst.c
@@ -7,7 +7,7 @@
#include <linux/uaccess.h>
#include <asm/inst.h>
-#ifdef __powerpc64__
+#ifdef CONFIG_PPC64
int probe_user_read_inst(struct ppc_inst *inst,
struct ppc_inst *nip)
{
@@ -17,9 +17,8 @@ int probe_user_read_inst(struct ppc_inst *inst,
err = probe_user_read(&val, nip, sizeof(val));
if (err)
return err;
- if ((val >> 26) == OP_PREFIX) {
- err = probe_user_read(&suffix, (void *)nip + 4,
- sizeof(unsigned int));
+ if (get_op(val) == OP_PREFIX) {
+ err = probe_user_read(&suffix, (void *)nip + 4, 4);
*inst = ppc_inst_prefix(val, suffix);
} else {
*inst = ppc_inst(val);
@@ -36,9 +35,8 @@ int probe_kernel_read_inst(struct ppc_inst *inst,
err = probe_kernel_read(&val, src, sizeof(val));
if (err)
return err;
- if ((val >> 26) == OP_PREFIX) {
- err = probe_kernel_read(&suffix, (void *)src + 4,
- sizeof(unsigned int));
+ if (get_op(val) == OP_PREFIX) {
+ err = probe_kernel_read(&suffix, (void *)src + 4, 4);
*inst = ppc_inst_prefix(val, suffix);
} else {
*inst = ppc_inst(val);
@@ -67,4 +65,4 @@ int probe_kernel_read_inst(struct ppc_inst *inst,
*inst = ppc_inst(val);
return err;
}
-#endif /* __powerpc64__ */
+#endif /* CONFIG_PPC64 */
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v8 24/30] powerpc: Test prefixed code patching
From: Jordan Niethe @ 2020-05-15 7:54 UTC (permalink / raw)
To: linuxppc-dev
Cc: Christophe Leroy, Alistair Popple, Nicholas Piggin, Balamuruhan S,
naveen.n.rao, Daniel Axtens
In-Reply-To: <20200506034050.24806-25-jniethe5@gmail.com>
Hey mpe could you add this please.
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -707,7 +707,7 @@ static void __init test_translate_branch(void)
vfree(buf);
}
-#ifdef __powerpc64__
+#ifdef CONFIG_PPC64
static void __init test_prefixed_patching(void)
{
extern unsigned int code_patching_test1[];
@@ -733,7 +733,7 @@ static int __init test_code_patching(void)
test_branch_bform();
test_create_function_call();
test_translate_branch();
-#ifdef __powerpc64__
+#ifdef CONFIG_PPC64
test_prefixed_patching();
#endif
--
2.17.1
^ permalink raw reply
* Re: [PATCH v8 25/30] powerpc: Test prefixed instructions in feature fixups
From: Jordan Niethe @ 2020-05-15 7:57 UTC (permalink / raw)
To: linuxppc-dev
Cc: Christophe Leroy, Alistair Popple, Nicholas Piggin, Balamuruhan S,
naveen.n.rao, Daniel Axtens
In-Reply-To: <20200506034050.24806-26-jniethe5@gmail.com>
Hey mpe, could you add this thanks.
diff --git a/arch/powerpc/lib/feature-fixups.c
b/arch/powerpc/lib/feature-fixups.c
--- a/arch/powerpc/lib/feature-fixups.c
+++ b/arch/powerpc/lib/feature-fixups.c
@@ -689,7 +689,7 @@ static void test_lwsync_macros(void)
}
}
-#ifdef __powerpc64__
+#ifdef CONFIG_PPC64
static void __init test_prefix_patching(void)
{
extern unsigned int ftr_fixup_prefix1[];
@@ -755,7 +755,7 @@ static void __init test_prefix_word_alt_patching(void)
patch_feature_section(0, &fixup);
check(memcmp(ftr_fixup_prefix3, ftr_fixup_prefix3_orig, size) != 0);
}
-#endif /* __powerpc64__ */
+#endif /* CONFIG_PPC64 */
static int __init test_feature_fixups(void)
{
@@ -771,7 +771,7 @@ static int __init test_feature_fixups(void)
test_cpu_macros();
test_fw_macros();
test_lwsync_macros();
-#ifdef __powerpc64__
+#ifdef CONFIG_PPC64
test_prefix_patching();
test_prefix_alt_patching();
test_prefix_word_alt_patching();
--
^ permalink raw reply
* Re: [PATCH v8 29/30] powerpc sstep: Add support for prefixed load/stores
From: Jordan Niethe @ 2020-05-15 7:59 UTC (permalink / raw)
To: linuxppc-dev
Cc: Christophe Leroy, Alistair Popple, Nicholas Piggin, Balamuruhan S,
naveen.n.rao, Daniel Axtens
In-Reply-To: <20200506034050.24806-30-jniethe5@gmail.com>
mpe, and this thanks.
----
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1204,7 +1204,7 @@ int analyse_instr(struct instruction_op *op,
const struct pt_regs *regs,
struct ppc_inst instr)
{
unsigned int opcode, ra, rb, rc, rd, spr, u;
-#ifdef __powerpc64__
+#ifdef CONFIG_PPC64
unsigned int suffixopcode, prefixtype, prefix_r;
#endif
unsigned long int imm;
@@ -2701,7 +2701,7 @@ int analyse_instr(struct instruction_op *op,
const struct pt_regs *regs,
op->reg = rd;
op->val = regs->gpr[rd];
- suffixopcode = suffix >> 26;
+ suffixopcode = get_op(suffix);
prefixtype = (word >> 24) & 0x3;
switch (prefixtype) {
case 0: /* Type 00 Eight-Byte Load/Store */
--
2.17.1
^ permalink raw reply
* Re: [PATCH v8 30/30] powerpc sstep: Add support for prefixed fixed-point arithmetic
From: Jordan Niethe @ 2020-05-15 8:02 UTC (permalink / raw)
To: linuxppc-dev
Cc: Christophe Leroy, Alistair Popple, Nicholas Piggin, Balamuruhan S,
naveen.n.rao, Daniel Axtens
In-Reply-To: <20200506034050.24806-31-jniethe5@gmail.com>
mpe, and this thanks.
---
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1343,7 +1343,7 @@ int analyse_instr(struct instruction_op *op,
const struct pt_regs *regs,
rd = (suffix >> 21) & 0x1f;
op->reg = rd;
op->val = regs->gpr[rd];
- suffixopcode = suffix >> 26;
+ suffixopcode = get_op(suffix);
prefixtype = (word >> 24) & 0x3;
switch (prefixtype) {
case 2:
--
2.17.1
^ permalink raw reply
* Re: [PATCH 2/3] ASoC: fsl_esai: Add support for imx8qm
From: Shengjiu Wang @ 2020-05-15 10:01 UTC (permalink / raw)
To: Mark Brown
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux-ALSA, Timur Tabi, Xiubo Li, Fabio Estevam, Shengjiu Wang,
Liam Girdwood, Takashi Iwai, Nicolin Chen, Rob Herring,
linuxppc-dev, linux-kernel
In-Reply-To: <20200512123801.GG5110@sirena.org.uk>
On Tue, May 12, 2020 at 8:38 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, May 12, 2020 at 10:48:41AM +0800, Shengjiu Wang wrote:
> > On Wed, May 6, 2020 at 10:33 AM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
> > > On Fri, May 1, 2020 at 6:23 PM Mark Brown <broonie@kernel.org> wrote:
>
> > > > > EDMA requires the period size to be multiple of maxburst. Otherwise
> > > > > the remaining bytes are not transferred and thus noise is produced.
>
> > > > If this constraint comes from the DMA controller then normally you'd
> > > > expect the DMA controller integration to be enforcing this - is there no
> > > > information in the DMA API that lets us know that this constraint is
> > > > there?
>
> > > No, I can't find one API for this.
> > > Do you have a recommendation?
>
> > could you please recommend which DMA API can I use?
>
> Not off-hand, you'd probably need to extend the API to export the
> information.
Thanks. I will think about if I can find a better solution.
And I will drop this change and send v2 of this patch-set.
^ permalink raw reply
* [PATCH v2 1/2] ASoC: fsl_esai: introduce SoC specific data
From: Shengjiu Wang @ 2020-05-15 10:10 UTC (permalink / raw)
To: timur, nicoleotsuka, Xiubo.Lee, festevam, broonie, perex, tiwai,
alsa-devel, lgirdwood, robh+dt, devicetree
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1589537601.git.shengjiu.wang@nxp.com>
Introduce a SoC specific data structure which contains the
differences between the different SoCs.
This makes it easier to support more differences without having
to introduce a new if/else each time.
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Acked-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
sound/soc/fsl/fsl_esai.c | 46 ++++++++++++++++++++++++++++++++--------
1 file changed, 37 insertions(+), 9 deletions(-)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index 84290be778f0..bac65ba7fbad 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -21,6 +21,17 @@
SNDRV_PCM_FMTBIT_S20_3LE | \
SNDRV_PCM_FMTBIT_S24_LE)
+/**
+ * fsl_esai_soc_data: soc specific data
+ *
+ * @imx: for imx platform
+ * @reset_at_xrun: flags for enable reset operaton
+ */
+struct fsl_esai_soc_data {
+ bool imx;
+ bool reset_at_xrun;
+};
+
/**
* fsl_esai: ESAI private data
*
@@ -33,6 +44,7 @@
* @fsysclk: system clock source to derive HCK, SCK and FS
* @spbaclk: SPBA clock (optional, depending on SoC design)
* @task: tasklet to handle the reset operation
+ * @soc: soc specific data
* @lock: spin lock between hw_reset() and trigger()
* @fifo_depth: depth of tx/rx FIFO
* @slot_width: width of each DAI slot
@@ -44,7 +56,6 @@
* @sck_div: if using PSR/PM dividers for SCKx clock
* @slave_mode: if fully using DAI slave mode
* @synchronous: if using tx/rx synchronous mode
- * @reset_at_xrun: flags for enable reset operaton
* @name: driver name
*/
struct fsl_esai {
@@ -57,6 +68,7 @@ struct fsl_esai {
struct clk *fsysclk;
struct clk *spbaclk;
struct tasklet_struct task;
+ const struct fsl_esai_soc_data *soc;
spinlock_t lock; /* Protect hw_reset and trigger */
u32 fifo_depth;
u32 slot_width;
@@ -70,10 +82,24 @@ struct fsl_esai {
bool sck_div[2];
bool slave_mode;
bool synchronous;
- bool reset_at_xrun;
char name[32];
};
+static struct fsl_esai_soc_data fsl_esai_vf610 = {
+ .imx = false,
+ .reset_at_xrun = true,
+};
+
+static struct fsl_esai_soc_data fsl_esai_imx35 = {
+ .imx = true,
+ .reset_at_xrun = true,
+};
+
+static struct fsl_esai_soc_data fsl_esai_imx6ull = {
+ .imx = true,
+ .reset_at_xrun = false,
+};
+
static irqreturn_t esai_isr(int irq, void *devid)
{
struct fsl_esai *esai_priv = (struct fsl_esai *)devid;
@@ -85,7 +111,7 @@ static irqreturn_t esai_isr(int irq, void *devid)
regmap_read(esai_priv->regmap, REG_ESAI_SAISR, &saisr);
if ((saisr & (ESAI_SAISR_TUE | ESAI_SAISR_ROE)) &&
- esai_priv->reset_at_xrun) {
+ esai_priv->soc->reset_at_xrun) {
dev_dbg(&pdev->dev, "reset module for xrun\n");
regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
ESAI_xCR_xEIE_MASK, 0);
@@ -936,9 +962,11 @@ static int fsl_esai_probe(struct platform_device *pdev)
esai_priv->pdev = pdev;
snprintf(esai_priv->name, sizeof(esai_priv->name), "%pOFn", np);
- if (of_device_is_compatible(np, "fsl,vf610-esai") ||
- of_device_is_compatible(np, "fsl,imx35-esai"))
- esai_priv->reset_at_xrun = true;
+ esai_priv->soc = of_device_get_match_data(&pdev->dev);
+ if (!esai_priv->soc) {
+ dev_err(&pdev->dev, "failed to get soc data\n");
+ return -ENODEV;
+ }
/* Get the addresses and IRQ */
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -1063,9 +1091,9 @@ static int fsl_esai_remove(struct platform_device *pdev)
}
static const struct of_device_id fsl_esai_dt_ids[] = {
- { .compatible = "fsl,imx35-esai", },
- { .compatible = "fsl,vf610-esai", },
- { .compatible = "fsl,imx6ull-esai", },
+ { .compatible = "fsl,imx35-esai", .data = &fsl_esai_imx35 },
+ { .compatible = "fsl,vf610-esai", .data = &fsl_esai_vf610 },
+ { .compatible = "fsl,imx6ull-esai", .data = &fsl_esai_imx6ull },
{}
};
MODULE_DEVICE_TABLE(of, fsl_esai_dt_ids);
--
2.21.0
^ permalink raw reply related
* [PATCH v2 0/2] ASoC: fsl_esai: Add support for imx8qm
From: Shengjiu Wang @ 2020-05-15 10:10 UTC (permalink / raw)
To: timur, nicoleotsuka, Xiubo.Lee, festevam, broonie, perex, tiwai,
alsa-devel, lgirdwood, robh+dt, devicetree
Cc: linuxppc-dev, linux-kernel
Add support for imx8qm.
Shengjiu Wang (2):
ASoC: fsl_esai: introduce SoC specific data
ASoC: fsl_esai: Add new compatible string for imx8qm
Changes in v2
- drop the 0002 patch in v1, the dma relate limitation should
be done in dma driver, or define a new DMA API for it.
.../devicetree/bindings/sound/fsl,esai.txt | 1 +
sound/soc/fsl/fsl_esai.c | 46 +++++++++++++++----
2 files changed, 38 insertions(+), 9 deletions(-)
--
2.21.0
^ permalink raw reply
* [PATCH v2 2/2] ASoC: fsl_esai: Add new compatible string for imx8qm
From: Shengjiu Wang @ 2020-05-15 10:10 UTC (permalink / raw)
To: timur, nicoleotsuka, Xiubo.Lee, festevam, broonie, perex, tiwai,
alsa-devel, lgirdwood, robh+dt, devicetree
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1589537601.git.shengjiu.wang@nxp.com>
Add new compatible string "fsl,imx8qm-esai" in the binding document.
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Acked-by: Rob Herring <robh@kernel.org>
---
Documentation/devicetree/bindings/sound/fsl,esai.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/sound/fsl,esai.txt b/Documentation/devicetree/bindings/sound/fsl,esai.txt
index 0e6e2166f76c..0a2480aeecf0 100644
--- a/Documentation/devicetree/bindings/sound/fsl,esai.txt
+++ b/Documentation/devicetree/bindings/sound/fsl,esai.txt
@@ -12,6 +12,7 @@ Required properties:
"fsl,imx35-esai",
"fsl,vf610-esai",
"fsl,imx6ull-esai",
+ "fsl,imx8qm-esai",
- reg : Offset and length of the register set for the device.
--
2.21.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox