by Lars Poeschel

[permalink] [raw]

Subject: Re: [PATCH v6 5/7] nfc: pn533: add UART phy driver

On Thu, Aug 22, 2019 at 10:09:09AM +0000, [email protected] wrote:
> Hi Lars,
>
> On 20.08.2019 15:03, Lars Poeschel wrote:
> > This adds the UART phy interface for the pn533 driver.
> > The pn533 driver can be used through UART interface this way.
> > It is implemented as a serdev device.
> >
> > Cc: Johan Hovold <[email protected]>
> > Signed-off-by: Lars Poeschel <[email protected]>
> > ---
> > Changes in v6:
> > - Rebased the patch series on v5.3-rc5
> >
> > Changes in v5:
> > - Use the splitted pn53x_common_init and pn53x_register_nfc
> > and pn53x_common_clean and pn53x_unregister_nfc alike
> >
> > Changes in v4:
> > - SPDX-License-Identifier: GPL-2.0+
> > - Source code comments above refering items
> > - Error check for serdev_device_write's
> > - Change if (xxx == NULL) to if (!xxx)
> > - Remove device name from a dev_err
> > - move pn533_register in _probe a bit towards the end of _probe
> > - make use of newly added dev_up / dev_down phy_ops
> > - control send_wakeup variable from dev_up / dev_down
> >
> > Changes in v3:
> > - depend on SERIAL_DEV_BUS in Kconfig
> >
> > Changes in v2:
> > - switched from tty line discipline to serdev, resulting in many
> > simplifications
> > - SPDX License Identifier
> >
> > drivers/nfc/pn533/Kconfig | 11 ++
> > drivers/nfc/pn533/Makefile | 2 +
> > drivers/nfc/pn533/pn533.h | 8 +
> > drivers/nfc/pn533/uart.c | 316 +++++++++++++++++++++++++++++++++++++
> > 4 files changed, 337 insertions(+)
> > create mode 100644 drivers/nfc/pn533/uart.c
> >
> > diff --git a/drivers/nfc/pn533/Kconfig b/drivers/nfc/pn533/Kconfig
> > index f6d6b345ba0d..7fe1bbe26568 100644
> > --- a/drivers/nfc/pn533/Kconfig
> > +++ b/drivers/nfc/pn533/Kconfig
> > @@ -26,3 +26,14 @@ config NFC_PN533_I2C
> >
> > If you choose to build a module, it'll be called pn533_i2c.
> > Say N if unsure.
> > +
> > +config NFC_PN532_UART
> > + tristate "NFC PN532 device support (UART)"
> > + depends on SERIAL_DEV_BUS
> > + select NFC_PN533
> > + ---help---
> > + This module adds support for the NXP pn532 UART interface.
> > + Select this if your platform is using the UART bus.
> > +
> > + If you choose to build a module, it'll be called pn532_uart.
> > + Say N if unsure.
> > diff --git a/drivers/nfc/pn533/Makefile b/drivers/nfc/pn533/Makefile
> > index 43c25b4f9466..b9648337576f 100644
> > --- a/drivers/nfc/pn533/Makefile
> > +++ b/drivers/nfc/pn533/Makefile
> > @@ -4,7 +4,9 @@
> > #
> > pn533_usb-objs = usb.o
> > pn533_i2c-objs = i2c.o
> > +pn532_uart-objs = uart.o
> >
> > obj-$(CONFIG_NFC_PN533) += pn533.o
> > obj-$(CONFIG_NFC_PN533_USB) += pn533_usb.o
> > obj-$(CONFIG_NFC_PN533_I2C) += pn533_i2c.o
> > +obj-$(CONFIG_NFC_PN532_UART) += pn532_uart.o
> > diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
> > index 510ddebbd896..6541088fad73 100644
> > --- a/drivers/nfc/pn533/pn533.h
> > +++ b/drivers/nfc/pn533/pn533.h
> > @@ -43,6 +43,11 @@
> >
> > /* Preamble (1), SoPC (2), ACK Code (2), Postamble (1) */
> > #define PN533_STD_FRAME_ACK_SIZE 6
> > +/*
> > + * Preamble (1), SoPC (2), Packet Length (1), Packet Length Checksum (1),
> > + * Specific Application Level Error Code (1) , Postamble (1)
> > + */
> > +#define PN533_STD_ERROR_FRAME_SIZE 8
> > #define PN533_STD_FRAME_CHECKSUM(f) (f->data[f->datalen])
> > #define PN533_STD_FRAME_POSTAMBLE(f) (f->data[f->datalen + 1])
> > /* Half start code (3), LEN (4) should be 0xffff for extended frame */
> > @@ -84,6 +89,9 @@
> > #define PN533_CMD_MI_MASK 0x40
> > #define PN533_CMD_RET_SUCCESS 0x00
> >
> > +#define PN533_FRAME_DATALEN_ACK 0x00
> > +#define PN533_FRAME_DATALEN_ERROR 0x01
> > +#define PN533_FRAME_DATALEN_EXTENDED 0xFF
> >
> > enum pn533_protocol_type {
> > PN533_PROTO_REQ_ACK_RESP = 0,
> > diff --git a/drivers/nfc/pn533/uart.c b/drivers/nfc/pn533/uart.c
> > new file mode 100644
> > index 000000000000..f1cc2354a4fd
> > --- /dev/null
> > +++ b/drivers/nfc/pn533/uart.c
> > @@ -0,0 +1,316 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Driver for NXP PN532 NFC Chip - UART transport layer
> > + *
> > + * Copyright (C) 2018 Lemonage Software GmbH
> > + * Author: Lars P?schel <[email protected]>
> > + * All rights reserved.
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/nfc.h>
> > +#include <linux/netdevice.h>
> > +#include <linux/of.h>
> > +#include <linux/serdev.h>
> > +#include "pn533.h"
> > +
> > +#define PN532_UART_SKB_BUFF_LEN (PN533_CMD_DATAEXCH_DATA_MAXLEN * 2)
> > +
> > +enum send_wakeup {
> > + PN532_SEND_NO_WAKEUP = 0,
> > + PN532_SEND_WAKEUP,
> > + PN532_SEND_LAST_WAKEUP,
> > +};
> > +
> > +
> > +struct pn532_uart_phy {
> > + struct serdev_device *serdev;
> > + struct sk_buff *recv_skb;
> > + struct pn533 *priv;
> > + enum send_wakeup send_wakeup;
>
> Could there be any concurrency issues w/ regards to accessing this
> variable? I see it is accessed in pn532_uart_send_frame(), pn532_dev_up(),
> pn532_dev_down() which may be called from the following wq:
>
> INIT_WORK(&priv->mi_tm_rx_work, pn533_wq_tm_mi_recv);
>
> INIT_WORK(&priv->mi_tm_tx_work, pn533_wq_tm_mi_send);
>
> INIT_DELAYED_WORK(&priv->poll_work, pn533_wq_poll);
>
>
> and from net/nfc/core.c via dev_up()/dev_down().

Well, I spend some minutes thinking about this. There should be no real
problem. The code in pn533.c ensures, that commands are transmitted
sequencially. And it always is command - response. So if a command is
send, the driver waits for a response from the chip.
So pn532_uart_send_frame should not be called multiple times without
reaching at least serdev_device_write, but at this point the race is
already over.
There is one exception, this is the abort command. This command can be
sent without receiving a previous response. So there is the possibility
of a successful race.
The send_wakeup variable is used to control if we need to send a
wakeup request to the pn532 chip prior to the actual command we would
like to send.
Worst thing that I see could happen - if the race succeeds - is that we
send a wakeup to the chip that is propably not needed as it is already
awake. But this does not hurt as a wakeup send to the pn532 is
essentially a no-op if the chip is awake already. I could have
implemented it so, that a wakeup is sent in front of every command
without thinking and the driver would work.
The same is with pn532_dev_up. It could be that there is one wakeup sent
to much, but it does not hurt.
pn532_dev_down is not problematic I think.

To sum it up: There is maybe a very little probability, but it does
nothing bad. Question is now: Is it worth mutex'ing the send_wakeup
variable or can we leave it as-is ?

Thank you for your review, Claudiu.
Regards,
Lars

2019-08-26 10:43:55

by Claudiu Beznea

[permalink] [raw]

Subject: Re: [PATCH v6 5/7] nfc: pn533: add UART phy driver

Hi Lars,

On 23.08.2019 13:06, Lars Poeschel wrote:
> External E-Mail
>
>
> On Thu, Aug 22, 2019 at 10:09:09AM +0000, [email protected] wrote:
>> Hi Lars,
>>
>> On 20.08.2019 15:03, Lars Poeschel wrote:
>>> This adds the UART phy interface for the pn533 driver.
>>> The pn533 driver can be used through UART interface this way.
>>> It is implemented as a serdev device.
>>>
>>> Cc: Johan Hovold <[email protected]>
>>> Signed-off-by: Lars Poeschel <[email protected]>
>>> ---
>>> Changes in v6:
>>> - Rebased the patch series on v5.3-rc5
>>>
>>> Changes in v5:
>>> - Use the splitted pn53x_common_init and pn53x_register_nfc
>>> and pn53x_common_clean and pn53x_unregister_nfc alike
>>>
>>> Changes in v4:
>>> - SPDX-License-Identifier: GPL-2.0+
>>> - Source code comments above refering items
>>> - Error check for serdev_device_write's
>>> - Change if (xxx == NULL) to if (!xxx)
>>> - Remove device name from a dev_err
>>> - move pn533_register in _probe a bit towards the end of _probe
>>> - make use of newly added dev_up / dev_down phy_ops
>>> - control send_wakeup variable from dev_up / dev_down
>>>
>>> Changes in v3:
>>> - depend on SERIAL_DEV_BUS in Kconfig
>>>
>>> Changes in v2:
>>> - switched from tty line discipline to serdev, resulting in many
>>> simplifications
>>> - SPDX License Identifier
>>>
>>> drivers/nfc/pn533/Kconfig | 11 ++
>>> drivers/nfc/pn533/Makefile | 2 +
>>> drivers/nfc/pn533/pn533.h | 8 +
>>> drivers/nfc/pn533/uart.c | 316 +++++++++++++++++++++++++++++++++++++
>>> 4 files changed, 337 insertions(+)
>>> create mode 100644 drivers/nfc/pn533/uart.c
>>>
>>> diff --git a/drivers/nfc/pn533/Kconfig b/drivers/nfc/pn533/Kconfig
>>> index f6d6b345ba0d..7fe1bbe26568 100644
>>> --- a/drivers/nfc/pn533/Kconfig
>>> +++ b/drivers/nfc/pn533/Kconfig
>>> @@ -26,3 +26,14 @@ config NFC_PN533_I2C
>>>
>>> If you choose to build a module, it'll be called pn533_i2c.
>>> Say N if unsure.
>>> +
>>> +config NFC_PN532_UART
>>> + tristate "NFC PN532 device support (UART)"
>>> + depends on SERIAL_DEV_BUS
>>> + select NFC_PN533
>>> + ---help---
>>> + This module adds support for the NXP pn532 UART interface.
>>> + Select this if your platform is using the UART bus.
>>> +
>>> + If you choose to build a module, it'll be called pn532_uart.
>>> + Say N if unsure.
>>> diff --git a/drivers/nfc/pn533/Makefile b/drivers/nfc/pn533/Makefile
>>> index 43c25b4f9466..b9648337576f 100644
>>> --- a/drivers/nfc/pn533/Makefile
>>> +++ b/drivers/nfc/pn533/Makefile
>>> @@ -4,7 +4,9 @@
>>> #
>>> pn533_usb-objs = usb.o
>>> pn533_i2c-objs = i2c.o
>>> +pn532_uart-objs = uart.o
>>>
>>> obj-$(CONFIG_NFC_PN533) += pn533.o
>>> obj-$(CONFIG_NFC_PN533_USB) += pn533_usb.o
>>> obj-$(CONFIG_NFC_PN533_I2C) += pn533_i2c.o
>>> +obj-$(CONFIG_NFC_PN532_UART) += pn532_uart.o
>>> diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
>>> index 510ddebbd896..6541088fad73 100644
>>> --- a/drivers/nfc/pn533/pn533.h
>>> +++ b/drivers/nfc/pn533/pn533.h
>>> @@ -43,6 +43,11 @@
>>>
>>> /* Preamble (1), SoPC (2), ACK Code (2), Postamble (1) */
>>> #define PN533_STD_FRAME_ACK_SIZE 6
>>> +/*
>>> + * Preamble (1), SoPC (2), Packet Length (1), Packet Length Checksum (1),
>>> + * Specific Application Level Error Code (1) , Postamble (1)
>>> + */
>>> +#define PN533_STD_ERROR_FRAME_SIZE 8
>>> #define PN533_STD_FRAME_CHECKSUM(f) (f->data[f->datalen])
>>> #define PN533_STD_FRAME_POSTAMBLE(f) (f->data[f->datalen + 1])
>>> /* Half start code (3), LEN (4) should be 0xffff for extended frame */
>>> @@ -84,6 +89,9 @@
>>> #define PN533_CMD_MI_MASK 0x40
>>> #define PN533_CMD_RET_SUCCESS 0x00
>>>
>>> +#define PN533_FRAME_DATALEN_ACK 0x00
>>> +#define PN533_FRAME_DATALEN_ERROR 0x01
>>> +#define PN533_FRAME_DATALEN_EXTENDED 0xFF
>>>
>>> enum pn533_protocol_type {
>>> PN533_PROTO_REQ_ACK_RESP = 0,
>>> diff --git a/drivers/nfc/pn533/uart.c b/drivers/nfc/pn533/uart.c
>>> new file mode 100644
>>> index 000000000000..f1cc2354a4fd
>>> --- /dev/null
>>> +++ b/drivers/nfc/pn533/uart.c
>>> @@ -0,0 +1,316 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * Driver for NXP PN532 NFC Chip - UART transport layer
>>> + *
>>> + * Copyright (C) 2018 Lemonage Software GmbH
>>> + * Author: Lars Pöschel <[email protected]>
>>> + * All rights reserved.
>>> + */
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/nfc.h>
>>> +#include <linux/netdevice.h>
>>> +#include <linux/of.h>
>>> +#include <linux/serdev.h>
>>> +#include "pn533.h"
>>> +
>>> +#define PN532_UART_SKB_BUFF_LEN (PN533_CMD_DATAEXCH_DATA_MAXLEN * 2)
>>> +
>>> +enum send_wakeup {
>>> + PN532_SEND_NO_WAKEUP = 0,
>>> + PN532_SEND_WAKEUP,
>>> + PN532_SEND_LAST_WAKEUP,
>>> +};
>>> +
>>> +
>>> +struct pn532_uart_phy {
>>> + struct serdev_device *serdev;
>>> + struct sk_buff *recv_skb;
>>> + struct pn533 *priv;
>>> + enum send_wakeup send_wakeup;
>>
>> Could there be any concurrency issues w/ regards to accessing this
>> variable? I see it is accessed in pn532_uart_send_frame(), pn532_dev_up(),
>> pn532_dev_down() which may be called from the following wq:
>>
>> INIT_WORK(&priv->mi_tm_rx_work, pn533_wq_tm_mi_recv);
>>
>> INIT_WORK(&priv->mi_tm_tx_work, pn533_wq_tm_mi_send);
>>
>> INIT_DELAYED_WORK(&priv->poll_work, pn533_wq_poll);
>>
>>
>> and from net/nfc/core.c via dev_up()/dev_down().
>
> Well, I spend some minutes thinking about this. There should be no real
> problem. The code in pn533.c ensures, that commands are transmitted
> sequencially. And it always is command - response. So if a command is
> send, the driver waits for a response from the chip.
> So pn532_uart_send_frame should not be called multiple times without
> reaching at least serdev_device_write, but at this point the race is
> already over.
> There is one exception, this is the abort command. This command can be
> sent without receiving a previous response. So there is the possibility
> of a successful race.
> The send_wakeup variable is used to control if we need to send a
> wakeup request to the pn532 chip prior to the actual command we would
> like to send.
> Worst thing that I see could happen - if the race succeeds - is that we
> send a wakeup to the chip that is propably not needed as it is already
> awake. But this does not hurt as a wakeup send to the pn532 is
> essentially a no-op if the chip is awake already. I could have
> implemented it so, that a wakeup is sent in front of every command
> without thinking and the driver would work.
> The same is with pn532_dev_up. It could be that there is one wakeup sent
> to much, but it does not hurt.
> pn532_dev_down is not problematic I think.
>
> To sum it up: There is maybe a very little probability, but it does
> nothing bad. Question is now: Is it worth mutex'ing the send_wakeup
> variable or can we leave it as-is ?

Being so as you described above, I am for leaving it as is. Maybe, as you
wish, document this somewhere (e.g. a comment in the code), so that others
to be aware of this.

Thank you,
Claudiu Beznea

>
> Thank you for your review, Claudiu.
> Regards,
> Lars
>
>