Re: [PATCH V2] leds: trigger: Introduce an USB port trigger
HI Rob, I got problems following your objections, so it took me some time to go back to this. On 21 July 2016 at 22:42, Rob Herring wrote: > On Wed, Jul 20, 2016 at 10:06:23AM +0200, Rafał Miłecki wrote: >> On 20 July 2016 at 03:02, Rob Herring wrote: >> > On Fri, Jul 15, 2016 at 11:10:45PM +0200, Rafał Miłecki wrote: >> >> This commit adds a new trigger that can turn on LED when USB device gets >> >> connected to the USB port. This can be useful for various home routers >> >> that have USB port and a proper LED telling user a device is connected. >> >> >> >> Right now this trigger is usable with a proper DT only, there isn't a >> >> way to specify USB ports from user space. This may change in a future. >> >> >> >> Signed-off-by: Rafał Miłecki >> >> --- >> >> V2: The first version got support for specifying list of USB ports from >> >> user space only. There was a (big try &) discussion on adding DT >> >> support. It led to a pretty simple solution of comparing of_node of >> >> usb_device to of_node specified in usb-ports property. >> >> Since it appeared DT support may be simpler and non-DT a bit more >> >> complex, this version drops previous support for "ports" and >> >> "new_port" and focuses on DT only. The plan is to see if this >> >> solution with DT is OK, get it accepted and then work on non-DT. >> >> >> >> Felipe: if there won't be any objections I'd like to ask for your Ack. >> >> --- >> >> Documentation/devicetree/bindings/leds/common.txt | 11 ++ >> >> Documentation/leds/ledtrig-usbport.txt| 19 ++ >> >> drivers/leds/trigger/Kconfig | 8 + >> >> drivers/leds/trigger/Makefile | 1 + >> >> drivers/leds/trigger/ledtrig-usbport.c| 206 >> >> ++ >> >> 5 files changed, 245 insertions(+) >> >> create mode 100644 Documentation/leds/ledtrig-usbport.txt >> >> create mode 100644 drivers/leds/trigger/ledtrig-usbport.c >> >> >> >> diff --git a/Documentation/devicetree/bindings/leds/common.txt >> >> b/Documentation/devicetree/bindings/leds/common.txt >> >> index af10678..75536f7 100644 >> >> --- a/Documentation/devicetree/bindings/leds/common.txt >> >> +++ b/Documentation/devicetree/bindings/leds/common.txt >> >> @@ -50,6 +50,12 @@ property can be omitted. >> >> For controllers that have no configurable timeout the >> >> flash-max-timeout-us >> >> property can be omitted. >> >> >> >> +Trigger specific properties for child nodes: >> >> + >> >> +usbport trigger: >> >> +- usb-ports : List of USB ports that usbport should observed for turning >> >> on a >> >> + given LED. >> > >> > I think this should be more generic such that it could work for disk or >> > network LEDs too. Perhaps 'led-triggers = '? trigger is a bit of >> > a Linux name, but I haven't thought of anything better. Really, I'd >> > prefer the link in the other direction (e.g. port node have a 'leds" or >> > '*-leds' property), but that's maybe harder to parse. >> >> I was already thinking on this as I plan to add "netdev" trigger at >> some point in the future. I'd like to use "net-devices" for it (as a >> equivalent or "usb-ports"). >> >> However there is a problem with your idea of sharing "led-triggers" >> between triggers.. Consider device with generic purpose LED that >> should be controllable by a user. I believe we should let user switch >> it's state, e.g. with: >> echo usbport > trigger >> echo netdev > trigger >> with a shared property I don't see how we couldn't handle it properly. > > Well, the userspace interface and DT bindings are independent things. > You can't argue for what the DT binding should look like based on the > userspace interface. I don't understand that. It sounds like you don't want user to have control over a LED that was specified in DT. If I got it right, it sounds like a bad idea to me. It's a known case (in marketing, usage model) to allow user disable all LEDs (e.g. to don't disturb him during the night). That sounds like a valid usage for me, so I want user to be able to switch between triggers. And this is of course what is already supported by triggers and user space interface. With *current* triggers implementation you can do: cd /sys/class/leds/foo/ echo none > trigger echo timer > trigger So I'm trying to have trigger specific entry in order to support more than a single trigger. > Perhaps we need a "device trigger" where you echo the device to > be the trigger (similar to how bind works). If you have something to id > the device, then you can lookup its struct device and then its of_node > ptr. Are you describing mixing user space interface with DT interface at this point? Because echoing device sounds like doing some "echo foo > bar" in user space. If so, I didn't design setting trigger details from user space yet. I'm aware it'll require more discussion, so I left it fo later. > The other problem with your userspace interface is it only works if > the tr
Re: clocksource_watchdog causing scheduling of timers every second (was [v13] support "task_isolation" mode)
I tested this patch on 4.7 and confirm that irq_work does not occurs anymore on the isolated cpu. Thanks! I don't know of any utility to test the task isolation feature, so I started one: https://github.com/giraldeau/taskisol The script exp.sh runs the taskisol to test five different conditions, but some behavior is not the one I would expect. At startup, it does: - register a custom signal handler for SIGUSR1 - sched_setaffinity() on CPU 1, which is isolated - mlockall(MCL_CURRENT) to prevent undesired page faults The default strict mode is set with: prctl(PR_SET_TASK_ISOLATION, PR_TASK_ISOLATION_ENABLE) And then, the syscall write() is called. From previous discussion, the SIGKILL should be sent, but it does not occur. When instead of calling write() we force a page fault, then the SIGKILL is correctly sent. When instead a custom signal handler SIGUSR1: prctl(PR_SET_TASK_ISOLATION, PR_TASK_ISOLATION_USERSIG | PR_TASK_ISOLATION_SET_SIG(SIGUSR1) The signal is never delivered, either when the syscall is issued nor when the page fault occurs. I can confirm that, if two taskisol are created on the same CPU, the second one fails with Resource temporarily unavailable, so that's fine. I can add more test cases depending on your comments, such as the TLB events triggered by another thread on a non-isolated core. But maybe there is already a test suite? Francis 2016-07-27 15:58 GMT-04:00 Chris Metcalf : > On 7/27/2016 3:53 PM, Christoph Lameter wrote: >> >> On Wed, 27 Jul 2016, Chris Metcalf wrote: >> >>> Looks good. Did you omit the equivalent fix in >>> clocksource_start_watchdog() >>> on purpose? For now I just took your change, but tweaked it to add the >>> equivalent diff with cpumask_first_and() there. >> >> Can the watchdog be started on an isolated cpu at all? I would expect that >> the code would start a watchdog only on a housekeeping cpu. > > > The code just starts the watchdog initially on the first online cpu. > In principle you could have configured that as an isolated cpu, so > without any change to that code, you'd interrupt that cpu. > > I guess another way to slice it would be to start the watchdog on the > current core. But just using the same idiom as in clocksource_watchdog() > seems cleanest to me. > > I added your patch to the series and pushed it up (along with adding your > Tested-by to the x86 enablement commit). It's still based on 4.6 so I'll > need > to rebase it once the merge window closes. > > > -- > Chris Metcalf, Mellanox Technologies > http://www.mellanox.com > -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/8] thunderbolt: Communication with the ICM (firmware)
On Thu, 28 Jul 2016 11:15:17 +0300 Amir Levy wrote: > +static LIST_HEAD(controllers_list); > +static DECLARE_RWSEM(controllers_list_rwsem); Why use a semaphore when simple spinlock or mutex would be better? -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/8] thunderbolt: Communication with the ICM (firmware)
On Thu, 28 Jul 2016 11:15:17 +0300
Amir Levy wrote:
> +int nhi_send_message(struct tbt_nhi_ctxt *nhi_ctxt, enum pdf_value pdf,
> + u32 msg_len, const u8 *msg, bool ignore_icm_resp)
> +{
Why not make msg a void * and not have to do so many casts?
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 6/8] thunderbolt: Networking transmit and receive
On Thu, 28 Jul 2016 11:15:19 +0300
Amir Levy wrote:
> + /* pad short packets */
> + if (unlikely(skb->len < ETH_ZLEN)) {
> + int pad_len = ETH_ZLEN - skb->len;
> +
> + /* The skb is freed on error */
> + if (unlikely(skb_pad(skb, pad_len))) {
> + cleaned_count += frame_count;
> + continue;
> + }
> + __skb_put(skb, pad_len);
> + }
Packets should be padded on transmit, not on receive??
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: clocksource_watchdog causing scheduling of timers every second (was [v13] support "task_isolation" mode)
On 7/29/2016 2:31 PM, Francis Giraldeau wrote:
I tested this patch on 4.7 and confirm that irq_work does not occurs anymore on
the isolated cpu. Thanks!
Great! Let me know if you'd like me to add your Tested-by in the patch series.
I don't know of any utility to test the task isolation feature, so I started
one:
https://github.com/giraldeau/taskisol
The script exp.sh runs the taskisol to test five different conditions, but some
behavior is not the one I would expect.
At startup, it does:
- register a custom signal handler for SIGUSR1
- sched_setaffinity() on CPU 1, which is isolated
- mlockall(MCL_CURRENT) to prevent undesired page faults
The default strict mode is set with:
prctl(PR_SET_TASK_ISOLATION, PR_TASK_ISOLATION_ENABLE)
And then, the syscall write() is called. From previous discussion, the SIGKILL
should be sent, but it does not occur. When instead of calling write() we force
a page fault, then the SIGKILL is correctly sent.
This looks like it may be a bug in the x86-specific part of the kernel support.
On tilegx and arm64, running your test does the right thing:
# ./taskisol default syscall
taskisol run
taskisol/1855: task_isolation mode lost due to syscall 64
Killed
I think the x86 support doesn't properly return right away from a bad
syscall. The patch below should fix that; can you try it? However, it's
not clear to me why the signal isn't getting delivered. Perhaps you can
try adding some tracing to the syscall_trace_enter() path and see if we're
actually running this code as expected? Thank you! :-)
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -90,8 +90,10 @@ unsigned long syscall_trace_enter_phase1(struct pt_regs
*regs, u32 arch)
/* In isolation mode, we may prevent the syscall from running. */
if (work & _TIF_TASK_ISOLATION) {
-if (task_isolation_syscall(regs->orig_ax) == -1)
-return -1;
+if (task_isolation_syscall(regs->orig_ax) == -1) {
+regs->orig_ax = -1;
+return 0;
+}
work &= ~_TIF_TASK_ISOLATION;
}
I updated my dataplane branch on kernel.org with this fix.
When instead a custom signal handler SIGUSR1:
prctl(PR_SET_TASK_ISOLATION, PR_TASK_ISOLATION_USERSIG |
PR_TASK_ISOLATION_SET_SIG(SIGUSR1)
The signal is never delivered, either when the syscall is issued nor when the
page fault occurs.
This is a bug in your test program. Try again with this fix:
--- a/taskisol.c
+++ b/taskisol.c
@@ -79,8 +79,9 @@ int main(int argc, char *argv[])
* The program completes when using USERSIG,
* but actually no signal is delivered
*/
-if (strcmp(argv[1], "signal") == 0) {
-if (prctl(PR_SET_TASK_ISOLATION, PR_TASK_ISOLATION_USERSIG |
+else if (strcmp(argv[1], "signal") == 0) {
+if (prctl(PR_SET_TASK_ISOLATION, PR_TASK_ISOLATION_ENABLE |
+ PR_TASK_ISOLATION_USERSIG |
PR_TASK_ISOLATION_SET_SIG(SIGUSR1)) < 0) {
perror("prctl sigusr");
return -1;
The prctl() API is intended to be one-shot, i.e. you set all the state you
want with a single prctl(). The next call to prctl() will reset the state
to whatever you specify (including if you don't specify "enable").
(Also, as a side note, I'd expect your Makefile to invoke $(CC) for taskisol,
not $(CXX) - there doesn't seem to be any actual C++ in the program.)
I can confirm that, if two taskisol are created on the same CPU, the second one
fails with Resource temporarily unavailable, so that's fine.
I can add more test cases depending on your comments, such as the TLB events
triggered by another thread on a non-isolated core. But maybe there is already
a test suite?
The appended code is what I've been using as a test harness. It passes on
tilegx and arm64. No guarantees as to production-level code quality :-)
#define _GNU_SOURCE
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#ifndef PR_SET_TASK_ISOLATION // Not in system headers yet?
# define PR_SET_TASK_ISOLATION 48
# define PR_GET_TASK_ISOLATION 49
# define PR_TASK_ISOLATION_ENABLE (1 << 0)
# define PR_TASK_ISOLATION_USERSIG (1 << 1)
# define PR_TASK_ISOLATION_SET_SIG(sig) (((sig) & 0x7f) << 8)
# define PR_TASK_ISOLATION_GET_SIG(bits) (((bits) >> 8) & 0x7f)
# define PR_TASK_ISOLATION_NOSIG \
(PR_TASK_ISOLATION_USERSIG | PR_TASK_ISOLATION_SET_SIG(0))
#endif
// The cpu we are using for isolation tests.
static int task_isolation_cpu;
// Overall status, maintained as tests run.
static int exit_status = EXIT_SUCCESS;
// Set affinity to a single cpu.
int set_my_cpu(int cpu)
{
cpu_set_t set;
CPU_ZERO(&set);
CPU_SET(cpu, &set);
return sched_setaffinity(0, sizeof(cpu_set_t), &set);
}
// Run a child process in task isolation mode and repo
Re: [PATCH v5 4/8] thunderbolt: Communication with the ICM (firmware)
On Fri, Jul 29, 2016 at 02:02:24PM -0700, Stephen Hemminger wrote: > On Thu, 28 Jul 2016 11:15:17 +0300 > Amir Levy wrote: > > > +static LIST_HEAD(controllers_list); > > +static DECLARE_RWSEM(controllers_list_rwsem); > > Why use a semaphore when simple spinlock or mutex would be better? And never use a RW semaphore unless you can benchmark the difference from a normal lock. If you can't benchmark it, then don't use it... -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
