linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH anybus v1 3/4] bus: support HMS Anybus-S bus.
@ 2018-10-25 15:20 thesven73
  2018-10-30  9:55 ` Linus Walleij
  0 siblings, 1 reply; 6+ messages in thread
From: thesven73 @ 2018-10-25 15:20 UTC (permalink / raw)
  To: svendev, lee.jones, robh+dt, mark.rutland, afaerber, treding,
	david, noralf, johan, monstr, michal.vokac, arnd, gregkh,
	john.garry, geert+renesas, robin.murphy, paul.gortmaker,
	sebastien.bourdelin, icenowy, yuanzhichang, stuyoder,
	linus.walleij, maxime.ripard, bogdan.purcareata
  Cc: linux-kernel, devicetree

Hi Linus, thanks a million for the detailed patch review, you've given this
patch a lot more attention than I was expecting. Great !!

> What you need to add is a bit of how the driver is architected.

I agree. Written once, read/maintained 100s of times, right?

> This is really needed because the driver is starting threads and
> running completions and tasks and whatnot to the left and
> right, and when random people come in to maintain this code they
> will be puzzled. 

I had originally architected this driver to be much simpler, with everything
running in the context of the userspace threads (except obviously the
interrupt). But it stood zero chance, the presence of the soft interrupt + the
h/w requirement to lock/unlock the region when acking the soft interrupt meant
that there were circular locking dependencies that always resulted in
deadlock :(

This single-thread message-queue architecture is harder to understand,
but much easier and safer in terms of synchronization.

I find it hard myself to keep track of functions that run in userland thread
context, and those that run in the kernel thread. Should I prefix kernel thread
functions with kt_* just to keep them apart?

> also "buss" is a germanism isn't it?  It should be just "anybus"?

There are several different types of anybus:
Anybus-M
Anybus-S
Anybus-CompactCOM

This driver implements Anybus-S only. I had originally prefixed files and
functions with anybus-s and anybus_s respectively, but it looked horrible
visually:

struct anybus_s_host *cd = data;
drivers/bus/anybus-s-host.c
include/linux/anybus-s-client.h

I'm completely open to suggestions on this one.
anybuss?
anybus-s?
just anybus?

>> +static irqreturn_t irq_handler(int irq, void *data)
>> +{
>> +       struct anybuss_host *cd = data;
>> +       int ind_ab;
>> +
>> +       /* reading the anybus indicator register acks the interrupt */
>> +       ind_ab = read_ind_ab(cd->regmap);
>> +       if (ind_ab < 0)
>> +               return IRQ_NONE;
>> +       atomic_set(&cd->ind_ab, ind_ab);
>> +       complete(&cd->card_boot);
>> +       wake_up(&cd->wq);
>> +       return IRQ_HANDLED;
>> +}

> It looks a bit like you reinvent threaded interrupts but enlighten me
> on the architecture and I might be able to get it.

HMS Industrial Networks designed the anybus interrupt line to be dual purpose.
In addition to being a 'real' interrupt during normal operation, it also signals
that the card has initialized when coming out of reset. As this is a one-time
thing, it needs a 'struct completion', not a wait_queue.

It is of course possible to emulate a struct completion using a waitqueue and
an atomic variable, but wasn't struct completion invented to eliminate the
subtle dangers of this?

So this is why the irq_handler has to call both complete() and wake_up(), so
it can't be modelled by a threaded interrupt.

Maybe if we use two separate irq_handlers: put the first one in place during
initialization, then when the card is initialized, remove it and put a threaded
one in place? Would this be a bit too complicated?

> This looks complex. Why can't you just sleep() and then
> retry this instead of shuffleing around different "tasks"?
> Are you actually modeling a state machine? In that case
> I can kind of understand it, then you just need one
> thread/work and assign it an enum of states or
> something, maybe name that "state" rather than task
> so we see what is going on.

Yes, I am modeling a state machine.
When userspace asks the anybus to do something, it throws a task into a queue,
and waits on the completion of that task.
The anybus processes the tasks sequentially, each task will go through
multiple states before completing:

userspace processes 	A	B	C
			|	|	|
			v	v	v
			-----------------
			| task waiting	|
			| task waiting	|
			| task waiting	|
			|---------------|
			| task running	|
			-----------------
				^
				|
			-----------------
			| anybus process |
			| single-thread	 |
			| h/w access	 |
			------------------

There is only one single kernel thread that accesses the hardware and the queue.
This prevents various subtle synchronization/deadlock issues related to the
soft interrupts.

The tasks change state by re-assigning their own task_fn callback:

function do_state_1(self) {
	...
	if (need state 2)
		self->task_fn = do_state_2;
}

function do_state_2(self) {
	...
	if (need_state_1)
		self->task_fn = do_state_1;
}

I could have opted for a classic state machine in a single callback:

function do_state(self) {
	switch (self->state) {
	case state_1:
		...
		if (need_state_2)
			self->state = state_2;
		break;
	case state_2:
		...
		if (need_state_1)
			self->state = state_1;
		break;
	}
}

But the former seemed easier to understand.
Obviously it's more important that it's easy to understand not to me, but to
most developers who read the code. So tell me if the callback approach is
too exotic.

>> +static void softint_ack(struct anybuss_host *cd)
>> +static void process_softint(struct anybuss_host *cd)
>
> This looks like MSI (message signalled interrupt) and makes me think
> that you should probably involve the irqchip maintainers. Interrupts
> shall be represented in the irqchip abstraction.

This is not a *real* software interrupt - it's just a bit in an internal
register that gets set by the anybus on a certain condition, and needs
to be ACKed. When the bit is set, the bus driver then tells userspace
about it - anyone who is running poll/select on the sysfs node or devnode.

The anybus documentation calls this a 'software interrupt'.

>> +       cd->reset = pdata->reset;
>
> This callback thing to handle reset doesn't seem right.

I agree, and I've gone through the exact same thought process before.

Right now I'm using platform_data for the bus driver's dependencies:
(the irq is passed out-of-band, via platform_get_resource())

+/**
+ * Platform data of the Anybus-S host controller.
+ *
+ * @regmap: provides access to the card dpram.
+ *		MUST NOT use caching
+ *		MUST NOT sleep
+ * @reset:  controls the card reset line.
+ */
+struct anybuss_host_pdata {
+	struct regmap *regmap;
+	anybuss_reset_t reset;
+};

Now imagine that the underlying anybus bridge is defined as a reset controller.
I could not find a way to pass a reset controller handle through platform_data.
It seemed possible via the devicetree only. I was developing on 4.9 at the time.

So what if we pass the dependencies not via platform_data, but via the
devicetree? In that case, I cannot find a way to pass the struct regmap
via the devicetree...

Wait... are you talking about
reset_controller_add_lookup() / devm_reset_control_get_exclusive() ?
That's new to me, and only used in a single driver right now. Would that work?

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH anybus v1 0/4] Support HMS Profinet Card over Anybus
@ 2018-10-24 14:24 Sven Van Asbroeck
  2018-10-24 14:24 ` [PATCH anybus v1 3/4] bus: support HMS Anybus-S bus Sven Van Asbroeck
  0 siblings, 1 reply; 6+ messages in thread
From: Sven Van Asbroeck @ 2018-10-24 14:24 UTC (permalink / raw)
  To: svendev, lee.jones, robh+dt, mark.rutland, afaerber, treding,
	david, noralf, johan, monstr, michal.vokac, arnd, gregkh,
	john.garry, andriy.shevchenko, geert+renesas, robin.murphy,
	paul.gortmaker, sebastien.bourdelin, icenowy, yuanzhichang,
	stuyoder, linus.walleij, maxime.ripard, bogdan.purcareata
  Cc: linux-kernel, devicetree

This patch set adds support for the HMS Industrial Networks AB Profinet card.

Profinet is an industry technical standard for data communication over
Industrial Ethernet, designed for collecting data from, and controlling,
equipment in industrial systems, with a particular strength in delivering data
under tight time constraints (on the order of 1ms or less).

The profinet card itself is connected to the system via an industrial bus
called 'anybus'.

I have followed the bus driver/client driver pattern, and created an anybus
bus driver, plus a client driver for the profinet card.

In case this patch set gets (eventually) accepted, drivers for other anybus
client cards may follow: flnet, cc-link, ...

The anybus slot on the host is located on an 'anybus bridge', which is
custom h/w designed by Arcx. Its driver is modeled as an mfd, which
instantiates two anybus slots.

v1:
	first shot

Sven Van Asbroeck (4):
  mfd: support the Arcx anybus bridge.
  dt-bindings: anybus-bridge: document devicetree binding.
  bus: support HMS Anybus-S bus.
  misc: support HMS Profinet IRT industrial controller.

 .../bindings/mfd/arcx,anybus-bridge.txt       |   37 +
 .../devicetree/bindings/vendor-prefixes.txt   |    1 +
 drivers/bus/Kconfig                           |   11 +
 drivers/bus/Makefile                          |    1 +
 drivers/bus/anybuss-host.c                    | 1301 +++++++++++++++++
 drivers/mfd/Kconfig                           |   11 +
 drivers/mfd/Makefile                          |    1 +
 drivers/mfd/anybus-bridge.c                   |  441 ++++++
 drivers/misc/Kconfig                          |   11 +
 drivers/misc/Makefile                         |    1 +
 drivers/misc/hms-profinet.c                   |  747 ++++++++++
 include/linux/anybuss-client.h                |  100 ++
 include/linux/anybuss-host.h                  |   28 +
 include/uapi/linux/hms-common.h               |   14 +
 include/uapi/linux/hms-profinet.h             |  101 ++
 15 files changed, 2806 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt
 create mode 100644 drivers/bus/anybuss-host.c
 create mode 100644 drivers/mfd/anybus-bridge.c
 create mode 100644 drivers/misc/hms-profinet.c
 create mode 100644 include/linux/anybuss-client.h
 create mode 100644 include/linux/anybuss-host.h
 create mode 100644 include/uapi/linux/hms-common.h
 create mode 100644 include/uapi/linux/hms-profinet.h

-- 
2.17.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-10-30 13:50 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-25 15:20 [PATCH anybus v1 3/4] bus: support HMS Anybus-S bus thesven73
2018-10-30  9:55 ` Linus Walleij
2018-10-30 13:50   ` thesven73
  -- strict thread matches above, loose matches on Subject: below --
2018-10-24 14:24 [PATCH anybus v1 0/4] Support HMS Profinet Card over Anybus Sven Van Asbroeck
2018-10-24 14:24 ` [PATCH anybus v1 3/4] bus: support HMS Anybus-S bus Sven Van Asbroeck
2018-10-24 15:58   ` Randy Dunlap
2018-10-25 11:08   ` Linus Walleij

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).