Re: [PATCH v7 0/3] vduse: add support for networking devices

[Maxime Coquelin]

Hello Michael,

On 2/1/24 09:40, Michael S. Tsirkin wrote:

On Thu, Feb 01, 2024 at 09:34:11AM +0100, Maxime Coquelin wrote:

Hi Jason,

It looks like all patches got acked by you.
Any blocker to queue the series for next release?

Thanks,
Maxime


I think it's good enough at this point. Will put it in
linux-next shortly.



I fetched linux-next and it seems the series is not in yet.
Is there anything to be reworked on my side?

Thanks,
Maxime

Re: [PATCH] vduse: implement DMA sync callbacks

[Maxime Coquelin]

Hello Christoph,

On 2/20/24 10:01, Christoph Hellwig wrote:

On Mon, Feb 19, 2024 at 06:06:06PM +0100, Maxime Coquelin wrote:

Since commit 295525e29a5b ("virtio_net: merge dma
operations when filling mergeable buffers"), VDUSE device
require support for DMA's .sync_single_for_cpu() operation
as the memory is non-coherent between the device and CPU
because of the use of a bounce buffer.

This patch implements both .sync_single_for_cpu() and
sync_single_for_device() callbacks, and also skip bounce
buffer copies during DMA map and unmap operations if the
DMA_ATTR_SKIP_CPU_SYNC attribute is set to avoid extra
copies of the same buffer.


vduse really needs to get out of implementing fake DMA operations for
something that is not DMA.



I wasn't involved when vduse was initially submitted, I don't know if
any alternative was considered at that time.

Do you have something specific in mind we could do to have VDUSE not
implementing DMA ops, knowing other vDPA devices rely on DMA?

Thanks,
Maxime

[PATCH] vduse: implement DMA sync callbacks

[Maxime Coquelin]

Since commit 295525e29a5b ("virtio_net: merge dma
operations when filling mergeable buffers"), VDUSE device
require support for DMA's .sync_single_for_cpu() operation
as the memory is non-coherent between the device and CPU
because of the use of a bounce buffer.

This patch implements both .sync_single_for_cpu() and
.sync_single_for_device() callbacks, and also skip bounce
buffer copies during DMA map and unmap operations if the
DMA_ATTR_SKIP_CPU_SYNC attribute is set to avoid extra
copies of the same buffer.

Signed-off-by: Maxime Coquelin 
---
 drivers/vdpa/vdpa_user/iova_domain.c | 27 ---
 drivers/vdpa/vdpa_user/iova_domain.h |  8 
 drivers/vdpa/vdpa_user/vduse_dev.c   | 22 ++
 3 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/drivers/vdpa/vdpa_user/iova_domain.c 
b/drivers/vdpa/vdpa_user/iova_domain.c
index 5e4a77b9bae6..791d38d6284c 100644
--- a/drivers/vdpa/vdpa_user/iova_domain.c
+++ b/drivers/vdpa/vdpa_user/iova_domain.c
@@ -373,6 +373,26 @@ static void vduse_domain_free_iova(struct iova_domain 
*iovad,
free_iova_fast(iovad, iova >> shift, iova_len);
 }
 
+void vduse_domain_sync_single_for_device(struct vduse_iova_domain *domain,
+ dma_addr_t dma_addr, size_t size,
+ enum dma_data_direction dir)
+{
+   read_lock(>bounce_lock);
+   if (dir == DMA_TO_DEVICE || dir == DMA_BIDIRECTIONAL)
+   vduse_domain_bounce(domain, dma_addr, size, DMA_TO_DEVICE);
+   read_unlock(>bounce_lock);
+}
+
+void vduse_domain_sync_single_for_cpu(struct vduse_iova_domain *domain,
+ dma_addr_t dma_addr, size_t size,
+ enum dma_data_direction dir)
+{
+   read_lock(>bounce_lock);
+   if (dir == DMA_FROM_DEVICE || dir == DMA_BIDIRECTIONAL)
+   vduse_domain_bounce(domain, dma_addr, size, DMA_FROM_DEVICE);
+   read_unlock(>bounce_lock);
+}
+
 dma_addr_t vduse_domain_map_page(struct vduse_iova_domain *domain,
 struct page *page, unsigned long offset,
 size_t size, enum dma_data_direction dir,
@@ -393,7 +413,8 @@ dma_addr_t vduse_domain_map_page(struct vduse_iova_domain 
*domain,
if (vduse_domain_map_bounce_page(domain, (u64)iova, (u64)size, pa))
goto err_unlock;
 
-   if (dir == DMA_TO_DEVICE || dir == DMA_BIDIRECTIONAL)
+   if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC) &&
+   (dir == DMA_TO_DEVICE || dir == DMA_BIDIRECTIONAL))
vduse_domain_bounce(domain, iova, size, DMA_TO_DEVICE);
 
read_unlock(>bounce_lock);
@@ -411,9 +432,9 @@ void vduse_domain_unmap_page(struct vduse_iova_domain 
*domain,
 enum dma_data_direction dir, unsigned long attrs)
 {
struct iova_domain *iovad = >stream_iovad;
-
read_lock(>bounce_lock);
-   if (dir == DMA_FROM_DEVICE || dir == DMA_BIDIRECTIONAL)
+   if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC) &&
+   (dir == DMA_FROM_DEVICE || dir == DMA_BIDIRECTIONAL))
vduse_domain_bounce(domain, dma_addr, size, DMA_FROM_DEVICE);
 
vduse_domain_unmap_bounce_page(domain, (u64)dma_addr, (u64)size);
diff --git a/drivers/vdpa/vdpa_user/iova_domain.h 
b/drivers/vdpa/vdpa_user/iova_domain.h
index 173e979b84a9..f92f22a7267d 100644
--- a/drivers/vdpa/vdpa_user/iova_domain.h
+++ b/drivers/vdpa/vdpa_user/iova_domain.h
@@ -44,6 +44,14 @@ int vduse_domain_set_map(struct vduse_iova_domain *domain,
 void vduse_domain_clear_map(struct vduse_iova_domain *domain,
struct vhost_iotlb *iotlb);
 
+void vduse_domain_sync_single_for_device(struct vduse_iova_domain *domain,
+ dma_addr_t dma_addr, size_t size,
+ enum dma_data_direction dir);
+
+void vduse_domain_sync_single_for_cpu(struct vduse_iova_domain *domain,
+ dma_addr_t dma_addr, size_t size,
+ enum dma_data_direction dir);
+
 dma_addr_t vduse_domain_map_page(struct vduse_iova_domain *domain,
 struct page *page, unsigned long offset,
 size_t size, enum dma_data_direction dir,
diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c 
b/drivers/vdpa/vdpa_user/vduse_dev.c
index 1d24da79c399..75354ce186a1 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -798,6 +798,26 @@ static const struct vdpa_config_ops vduse_vdpa_config_ops 
= {
.free   = vduse_vdpa_free,
 };
 
+static void vduse_dev_sync_single_for_device(struct device *dev,
+dma_addr_t dma_addr, size_t size,
+enum dma_data_direct

Re: [PATCH v7 0/3] vduse: add support for networking devices

[Maxime Coquelin]

On 2/1/24 09:40, Michael S. Tsirkin wrote:

On Thu, Feb 01, 2024 at 09:34:11AM +0100, Maxime Coquelin wrote:

Hi Jason,

It looks like all patches got acked by you.
Any blocker to queue the series for next release?

Thanks,
Maxime


I think it's good enough at this point. Will put it in
linux-next shortly.



Thanks Michael!

Re: [PATCH v7 0/3] vduse: add support for networking devices

[Maxime Coquelin]

Hi Jason,

It looks like all patches got acked by you.
Any blocker to queue the series for next release?

Thanks,
Maxime

On 1/9/24 12:10, Maxime Coquelin wrote:

This small series enables virtio-net device type in VDUSE.
With it, basic operation have been tested, both with
virtio-vdpa and vhost-vdpa using DPDK Vhost library series
adding VDUSE support using split rings layout (merged in
DPDK v23.07-rc1).

Control queue support (and so multiqueue) has also been
tested, but requires a Kernel series from Jason Wang
relaxing control queue polling [1] to function reliably,
so while Jason rework is done, a patch is added to fail init
if control queue feature is requested.(tested also with DPDK
v23.11).


Changes in v7:
==
- Fail init only if VIRTIO_NET_F_CTRL_VQ is requested.
- Convert to use BIT_ULL() macro
- Rebased

Changes in v6:
==
- Remove SELinux support from the series, will be handled
   in a dedicated one.
- Require CAP_NET_ADMIN for Net devices creation (Jason).
- Fail init if control queue features are requested for
   Net device type (Jason).
- Rebased on latest master.

Changes in v5:
==
- Move control queue disablement patch before Net
   devices enablement (Jason).
- Unify operations LSM hooks into a single hook.
- Rebase on latest master.

Maxime Coquelin (3):
   vduse: validate block features only with block devices
   vduse: Temporarily fail if control queue feature requested
   vduse: enable Virtio-net device type

  drivers/vdpa/vdpa_user/vduse_dev.c | 24 
  1 file changed, 20 insertions(+), 4 deletions(-)

[PATCH v7 3/3] vduse: enable Virtio-net device type

[Maxime Coquelin]

This patch adds Virtio-net device type to the supported
devices types.

Initialization fails if the device does not support
VIRTIO_F_VERSION_1 feature, in order to guarantee the
configuration space is read-only. It also fails with
-EPERM if the CAP_NET_ADMIN is missing.

Acked-by: Jason Wang 
Reviewed-by: Eugenio Pérez 
Signed-off-by: Maxime Coquelin 
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c 
b/drivers/vdpa/vdpa_user/vduse_dev.c
index 00f3f562ab5d..8924bbc55635 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -143,6 +143,7 @@ static struct workqueue_struct *vduse_irq_bound_wq;
 
 static u32 allowed_device_id[] = {
VIRTIO_ID_BLOCK,
+   VIRTIO_ID_NET,
 };
 
 static inline struct vduse_dev *vdpa_to_vduse(struct vdpa_device *vdpa)
@@ -1686,6 +1687,10 @@ static bool features_is_valid(struct vduse_dev_config 
*config)
(config->features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
return false;
 
+   if ((config->device_id == VIRTIO_ID_NET) &&
+   !(config->features & BIT_ULL(VIRTIO_F_VERSION_1)))
+   return false;
+
return true;
 }
 
@@ -1793,6 +1798,10 @@ static int vduse_create_dev(struct vduse_dev_config 
*config,
int ret;
struct vduse_dev *dev;
 
+   ret = -EPERM;
+   if ((config->device_id == VIRTIO_ID_NET) && !capable(CAP_NET_ADMIN))
+   goto err;
+
ret = -EEXIST;
if (vduse_find_dev(config->name))
goto err;
@@ -2036,6 +2045,7 @@ static const struct vdpa_mgmtdev_ops vdpa_dev_mgmtdev_ops 
= {
 
 static struct virtio_device_id id_table[] = {
{ VIRTIO_ID_BLOCK, VIRTIO_DEV_ANY_ID },
+   { VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
{ 0 },
 };
 
-- 
2.43.0

[PATCH v7 2/3] vduse: Temporarily fail if control queue feature requested

[Maxime Coquelin]

Virtio-net driver control queue implementation is not safe
when used with VDUSE. If the VDUSE application does not
reply to control queue messages, it currently ends up
hanging the kernel thread sending this command.

Some work is on-going to make the control queue
implementation robust with VDUSE. Until it is completed,
let's fail features check if control-queue feature is
requested.

Signed-off-by: Maxime Coquelin 
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c 
b/drivers/vdpa/vdpa_user/vduse_dev.c
index a5af6d4077b8..00f3f562ab5d 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -8,6 +8,7 @@
  *
  */
 
+#include "linux/virtio_net.h"
 #include 
 #include 
 #include 
@@ -28,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "iova_domain.h"
@@ -1680,6 +1682,9 @@ static bool features_is_valid(struct vduse_dev_config 
*config)
if ((config->device_id == VIRTIO_ID_BLOCK) &&
(config->features & BIT_ULL(VIRTIO_BLK_F_CONFIG_WCE)))
return false;
+   else if ((config->device_id == VIRTIO_ID_NET) &&
+   (config->features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
+   return false;
 
return true;
 }
-- 
2.43.0

[PATCH v7 1/3] vduse: validate block features only with block devices

[Maxime Coquelin]

This patch is preliminary work to enable network device
type support to VDUSE.

As VIRTIO_BLK_F_CONFIG_WCE shares the same value as
VIRTIO_NET_F_HOST_TSO4, we need to restrict its check
to Virtio-blk device type.

Acked-by: Jason Wang 
Reviewed-by: Xie Yongji 
Reviewed-by: Eugenio Pérez 
Signed-off-by: Maxime Coquelin 
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c 
b/drivers/vdpa/vdpa_user/vduse_dev.c
index 0ddd4b8abecb..a5af6d4077b8 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -1671,13 +1671,14 @@ static bool device_is_allowed(u32 device_id)
return false;
 }
 
-static bool features_is_valid(u64 features)
+static bool features_is_valid(struct vduse_dev_config *config)
 {
-   if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
+   if (!(config->features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
return false;
 
/* Now we only support read-only configuration space */
-   if (features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE))
+   if ((config->device_id == VIRTIO_ID_BLOCK) &&
+   (config->features & BIT_ULL(VIRTIO_BLK_F_CONFIG_WCE)))
return false;
 
return true;
@@ -1704,7 +1705,7 @@ static bool vduse_validate_config(struct vduse_dev_config 
*config)
if (!device_is_allowed(config->device_id))
return false;
 
-   if (!features_is_valid(config->features))
+   if (!features_is_valid(config))
return false;
 
return true;
-- 
2.43.0

[PATCH v7 0/3] vduse: add support for networking devices

[Maxime Coquelin]

This small series enables virtio-net device type in VDUSE.
With it, basic operation have been tested, both with
virtio-vdpa and vhost-vdpa using DPDK Vhost library series
adding VDUSE support using split rings layout (merged in
DPDK v23.07-rc1).

Control queue support (and so multiqueue) has also been
tested, but requires a Kernel series from Jason Wang
relaxing control queue polling [1] to function reliably,
so while Jason rework is done, a patch is added to fail init
if control queue feature is requested.(tested also with DPDK
v23.11).


Changes in v7:
==
- Fail init only if VIRTIO_NET_F_CTRL_VQ is requested.
- Convert to use BIT_ULL() macro
- Rebased

Changes in v6:
==
- Remove SELinux support from the series, will be handled
  in a dedicated one.
- Require CAP_NET_ADMIN for Net devices creation (Jason).
- Fail init if control queue features are requested for
  Net device type (Jason).
- Rebased on latest master.

Changes in v5:
==
- Move control queue disablement patch before Net
  devices enablement (Jason).
- Unify operations LSM hooks into a single hook.
- Rebase on latest master.

Maxime Coquelin (3):
  vduse: validate block features only with block devices
  vduse: Temporarily fail if control queue feature requested
  vduse: enable Virtio-net device type

 drivers/vdpa/vdpa_user/vduse_dev.c | 24 
 1 file changed, 20 insertions(+), 4 deletions(-)

-- 
2.43.0

Re: [PATCH v6 2/3] vduse: Temporarily fail if control queue features requested

[Maxime Coquelin]

On 1/5/24 10:59, Eugenio Perez Martin wrote:

On Fri, Jan 5, 2024 at 9:12 AM Maxime Coquelin
 wrote:




On 1/5/24 03:45, Jason Wang wrote:

On Thu, Jan 4, 2024 at 11:38 PM Maxime Coquelin
 wrote:


Virtio-net driver control queue implementation is not safe
when used with VDUSE. If the VDUSE application does not
reply to control queue messages, it currently ends up
hanging the kernel thread sending this command.

Some work is on-going to make the control queue
implementation robust with VDUSE. Until it is completed,
let's fail features check if any control-queue related
feature is requested.

Signed-off-by: Maxime Coquelin 
---
   drivers/vdpa/vdpa_user/vduse_dev.c | 13 +
   1 file changed, 13 insertions(+)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c 
b/drivers/vdpa/vdpa_user/vduse_dev.c
index 0486ff672408..94f54ea2eb06 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -28,6 +28,7 @@
   #include 
   #include 
   #include 
+#include 
   #include 

   #include "iova_domain.h"
@@ -46,6 +47,15 @@

   #define IRQ_UNBOUND -1

+#define VDUSE_NET_INVALID_FEATURES_MASK \
+   (BIT_ULL(VIRTIO_NET_F_CTRL_VQ) |\
+BIT_ULL(VIRTIO_NET_F_CTRL_RX)   |  \
+BIT_ULL(VIRTIO_NET_F_CTRL_VLAN) |  \
+BIT_ULL(VIRTIO_NET_F_GUEST_ANNOUNCE) | \
+BIT_ULL(VIRTIO_NET_F_MQ) | \
+BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR) |  \
+BIT_ULL(VIRTIO_NET_F_RSS))


We need to make this as well:

VIRTIO_NET_F_CTRL_GUEST_OFFLOADS


I missed it, and see others have been added in the Virtio spec
repository (BTW, I see this specific one is missing in the dependency
list [0], I will submit a patch).

I wonder if it is not just simpler to just check for
VIRTIO_NET_F_CTRL_VQ is requested. As we fail instead of masking out,
the VDUSE driver won't be the one violating the spec so it should be
good?

It will avoid having to update the mask if new features depending on it
are added (or forgetting to update it).

WDYT?



I think it is safer to work with a whitelist, instead of a blacklist.
As any new feature might require code changes in QEMU. Is that
possible?


Well, that's how it was done in previous revision. :)

I changed to a blacklist for consistency with block device's WCE feature
check after Jason's comment.

I'm not sure moving back to a whitelist brings much advantages when
compared to the effort of keeping it up to date. Just blacklisting
VIRTIO_NET_F_CTRL_VQ is enough in my opinion.

Thanks,
Maxime


Thanks,
Maxime

[0]:
https://github.com/oasis-tcs/virtio-spec/blob/5fc35a7efb903fc352da81a6d2be5c01810b68d3/device-types/net/description.tex#L129

Other than this,

Acked-by: Jason Wang 

Thanks


+
   struct vduse_virtqueue {
  u16 index;
  u16 num_max;
@@ -1680,6 +1690,9 @@ static bool features_is_valid(struct vduse_dev_config 
*config)
  if ((config->device_id == VIRTIO_ID_BLOCK) &&
  (config->features & (1ULL << 
VIRTIO_BLK_F_CONFIG_WCE)))
  return false;
+   else if ((config->device_id == VIRTIO_ID_NET) &&
+   (config->features & VDUSE_NET_INVALID_FEATURES_MASK))
+   return false;

  return true;
   }
--
2.43.0

Re: [PATCH v6 2/3] vduse: Temporarily fail if control queue features requested

[Maxime Coquelin]

On 1/5/24 03:45, Jason Wang wrote:

On Thu, Jan 4, 2024 at 11:38 PM Maxime Coquelin
 wrote:


Virtio-net driver control queue implementation is not safe
when used with VDUSE. If the VDUSE application does not
reply to control queue messages, it currently ends up
hanging the kernel thread sending this command.

Some work is on-going to make the control queue
implementation robust with VDUSE. Until it is completed,
let's fail features check if any control-queue related
feature is requested.

Signed-off-by: Maxime Coquelin 
---
  drivers/vdpa/vdpa_user/vduse_dev.c | 13 +
  1 file changed, 13 insertions(+)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c 
b/drivers/vdpa/vdpa_user/vduse_dev.c
index 0486ff672408..94f54ea2eb06 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -28,6 +28,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 

  #include "iova_domain.h"
@@ -46,6 +47,15 @@

  #define IRQ_UNBOUND -1

+#define VDUSE_NET_INVALID_FEATURES_MASK \
+   (BIT_ULL(VIRTIO_NET_F_CTRL_VQ) |\
+BIT_ULL(VIRTIO_NET_F_CTRL_RX)   |  \
+BIT_ULL(VIRTIO_NET_F_CTRL_VLAN) |  \
+BIT_ULL(VIRTIO_NET_F_GUEST_ANNOUNCE) | \
+BIT_ULL(VIRTIO_NET_F_MQ) | \
+BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR) |  \
+BIT_ULL(VIRTIO_NET_F_RSS))


We need to make this as well:

VIRTIO_NET_F_CTRL_GUEST_OFFLOADS


I missed it, and see others have been added in the Virtio spec
repository (BTW, I see this specific one is missing in the dependency
list [0], I will submit a patch).

I wonder if it is not just simpler to just check for
VIRTIO_NET_F_CTRL_VQ is requested. As we fail instead of masking out,
the VDUSE driver won't be the one violating the spec so it should be
good?

It will avoid having to update the mask if new features depending on it
are added (or forgetting to update it).

WDYT?

Thanks,
Maxime

[0]: 
https://github.com/oasis-tcs/virtio-spec/blob/5fc35a7efb903fc352da81a6d2be5c01810b68d3/device-types/net/description.tex#L129

Other than this,

Acked-by: Jason Wang 

Thanks


+
  struct vduse_virtqueue {
 u16 index;
 u16 num_max;
@@ -1680,6 +1690,9 @@ static bool features_is_valid(struct vduse_dev_config 
*config)
 if ((config->device_id == VIRTIO_ID_BLOCK) &&
 (config->features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE)))
 return false;
+   else if ((config->device_id == VIRTIO_ID_NET) &&
+   (config->features & VDUSE_NET_INVALID_FEATURES_MASK))
+   return false;

 return true;
  }
--
2.43.0

[PATCH v6 3/3] vduse: enable Virtio-net device type

[Maxime Coquelin]

This patch adds Virtio-net device type to the supported
devices types.

Initialization fails if the device does not support
VIRTIO_F_VERSION_1 feature, in order to guarantee the
configuration space is read-only. It also fails with
-EPERM if the CAP_NET_ADMIN is missing.

Signed-off-by: Maxime Coquelin 
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c 
b/drivers/vdpa/vdpa_user/vduse_dev.c
index 94f54ea2eb06..4057b34ff995 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -151,6 +151,7 @@ static struct workqueue_struct *vduse_irq_bound_wq;
 
 static u32 allowed_device_id[] = {
VIRTIO_ID_BLOCK,
+   VIRTIO_ID_NET,
 };
 
 static inline struct vduse_dev *vdpa_to_vduse(struct vdpa_device *vdpa)
@@ -1694,6 +1695,10 @@ static bool features_is_valid(struct vduse_dev_config 
*config)
(config->features & VDUSE_NET_INVALID_FEATURES_MASK))
return false;
 
+   if ((config->device_id == VIRTIO_ID_NET) &&
+   !(config->features & (1ULL << VIRTIO_F_VERSION_1)))
+   return false;
+
return true;
 }
 
@@ -1801,6 +1806,10 @@ static int vduse_create_dev(struct vduse_dev_config 
*config,
int ret;
struct vduse_dev *dev;
 
+   ret = -EPERM;
+   if ((config->device_id == VIRTIO_ID_NET) && !capable(CAP_NET_ADMIN))
+   goto err;
+
ret = -EEXIST;
if (vduse_find_dev(config->name))
goto err;
@@ -2044,6 +2053,7 @@ static const struct vdpa_mgmtdev_ops vdpa_dev_mgmtdev_ops 
= {
 
 static struct virtio_device_id id_table[] = {
{ VIRTIO_ID_BLOCK, VIRTIO_DEV_ANY_ID },
+   { VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
{ 0 },
 };
 
-- 
2.43.0

[PATCH v6 2/3] vduse: Temporarily fail if control queue features requested

[Maxime Coquelin]

Virtio-net driver control queue implementation is not safe
when used with VDUSE. If the VDUSE application does not
reply to control queue messages, it currently ends up
hanging the kernel thread sending this command.

Some work is on-going to make the control queue
implementation robust with VDUSE. Until it is completed,
let's fail features check if any control-queue related
feature is requested.

Signed-off-by: Maxime Coquelin 
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c 
b/drivers/vdpa/vdpa_user/vduse_dev.c
index 0486ff672408..94f54ea2eb06 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "iova_domain.h"
@@ -46,6 +47,15 @@
 
 #define IRQ_UNBOUND -1
 
+#define VDUSE_NET_INVALID_FEATURES_MASK \
+   (BIT_ULL(VIRTIO_NET_F_CTRL_VQ) |\
+BIT_ULL(VIRTIO_NET_F_CTRL_RX)   |  \
+BIT_ULL(VIRTIO_NET_F_CTRL_VLAN) |  \
+BIT_ULL(VIRTIO_NET_F_GUEST_ANNOUNCE) | \
+BIT_ULL(VIRTIO_NET_F_MQ) | \
+BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR) |  \
+BIT_ULL(VIRTIO_NET_F_RSS))
+
 struct vduse_virtqueue {
u16 index;
u16 num_max;
@@ -1680,6 +1690,9 @@ static bool features_is_valid(struct vduse_dev_config 
*config)
if ((config->device_id == VIRTIO_ID_BLOCK) &&
(config->features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE)))
return false;
+   else if ((config->device_id == VIRTIO_ID_NET) &&
+   (config->features & VDUSE_NET_INVALID_FEATURES_MASK))
+   return false;
 
return true;
 }
-- 
2.43.0

[PATCH v6 1/3] vduse: validate block features only with block devices

[Maxime Coquelin]

This patch is preliminary work to enable network device
type support to VDUSE.

As VIRTIO_BLK_F_CONFIG_WCE shares the same value as
VIRTIO_NET_F_HOST_TSO4, we need to restrict its check
to Virtio-blk device type.

Acked-by: Jason Wang 
Reviewed-by: Xie Yongji 
Signed-off-by: Maxime Coquelin 
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c 
b/drivers/vdpa/vdpa_user/vduse_dev.c
index 0ddd4b8abecb..0486ff672408 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -1671,13 +1671,14 @@ static bool device_is_allowed(u32 device_id)
return false;
 }
 
-static bool features_is_valid(u64 features)
+static bool features_is_valid(struct vduse_dev_config *config)
 {
-   if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
+   if (!(config->features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
return false;
 
/* Now we only support read-only configuration space */
-   if (features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE))
+   if ((config->device_id == VIRTIO_ID_BLOCK) &&
+   (config->features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE)))
return false;
 
return true;
@@ -1704,7 +1705,7 @@ static bool vduse_validate_config(struct vduse_dev_config 
*config)
if (!device_is_allowed(config->device_id))
return false;
 
-   if (!features_is_valid(config->features))
+   if (!features_is_valid(config))
return false;
 
return true;
-- 
2.43.0

[PATCH v6 0/3] vduse: add support for networking devices

[Maxime Coquelin]

This small series enables virtio-net device type in VDUSE.
With it, basic operation have been tested, both with
virtio-vdpa and vhost-vdpa using DPDK Vhost library series
adding VDUSE support using split rings layout (merged in
DPDK v23.07-rc1).

Control queue support (and so multiqueue) has also been
tested, but requires a Kernel series from Jason Wang
relaxing control queue polling [1] to function reliably,
so while Jason rework is done, a patch is added to disable
CVQ and features that depend on it (tested also with DPDK
v23.07-rc1).

In this v5, LSM hooks introduced in previous revision are
unified into a single hook that covers below operations:
- VDUSE_CREATE_DEV ioctl on VDUSE control file,
- VDUSE_DESTROY_DEV ioctl on VDUSE control file,
- open() on VDUSE device file.

In combination with the operations permission, a device type
permission has to be associated:
- block: Virtio block device type,
- net: Virtio networking device type.

changes in v6!
==
- Remove SELinux support from the series, will be handled
  in a dedicated one.
- Require CAP_NET_ADMIN for Net devices creation (Jason).
- Fail init if control queue features are requested for
  Net device type (Jason).
- Rebased on latest master.

Changes in v5:
==
- Move control queue disablement patch before Net
  devices enablement (Jason).
- Unify operations LSM hooks into a single hook.
- Rebase on latest master.

Maxime Coquelin (3):
  vduse: validate block features only with block devices
  vduse: Temporarily fail if control queue features requested
  vduse: enable Virtio-net device type

 drivers/vdpa/vdpa_user/vduse_dev.c | 32 ++
 1 file changed, 28 insertions(+), 4 deletions(-)

-- 
2.43.0

Re: [PATCH v5 4/4] vduse: Add LSM hook to check Virtio device type

[Maxime Coquelin]

On 12/18/23 18:33, Stephen Smalley wrote:

On Mon, Dec 18, 2023 at 12:21 PM Stephen Smalley
 wrote:


On Tue, Dec 12, 2023 at 8:17 AM Maxime Coquelin
 wrote:


This patch introduces a LSM hook for devices creation,
destruction (ioctl()) and opening (open()) operations,
checking the application is allowed to perform these
operations for the Virtio device type.


Can you explain why the existing LSM hooks and SELinux implementation
are not sufficient? We already control the ability to open device
nodes via selinux_inode_permission() and selinux_file_open(), and can
support fine-grained per-cmd ioctl checking via selinux_file_ioctl().
And it should already be possible to label these nodes distinctly
through existing mechanisms (file_contexts if udev-created/labeled,
genfs_contexts if kernel-created). What exactly can't you do today
that this hook enables?


(added Ondrej to the distribution; IMHO we should swap him into
MAINTAINERS in place of Eric Paris since Eric has long-since moved on
from SELinux and Ondrej serves in that capacity these days)

Other items to consider:
- If vduse devices are created using anonymous inodes, then SELinux
grew a general facility for labeling and controlling the creation of
those via selinux_inode_init_security_anon().
- You can encode information about the device into its SELinux type
that then allows you to distinguish things like net vs block based on
the device's SELinux security context rather than encoding that in the
permission bits.


Got it, that seems indeed more appropriate than using persmission bits
for the device type.


- If you truly need new LSM hooks (which you need to prove first),
then you should pass some usable information about the object in
question to allow SELinux to find a security context for it. Like an
inode associated with the device, for example.


Ok.





Thanks for the insights, I'll try and see if I can follow your
recommendations in a dedicated series.

Maxime

Re: [PATCH v5 2/4] vduse: Temporarily disable control queue features

[Maxime Coquelin]

On 12/18/23 03:50, Jason Wang wrote:

On Wed, Dec 13, 2023 at 7:23 PM Maxime Coquelin
 wrote:


Hi Jason,

On 12/13/23 05:52, Jason Wang wrote:

On Tue, Dec 12, 2023 at 9:17 PM Maxime Coquelin
 wrote:


Virtio-net driver control queue implementation is not safe
when used with VDUSE. If the VDUSE application does not
reply to control queue messages, it currently ends up
hanging the kernel thread sending this command.

Some work is on-going to make the control queue
implementation robust with VDUSE. Until it is completed,
let's disable control virtqueue and features that depend on
it.

Signed-off-by: Maxime Coquelin 


I wonder if it's better to fail instead of a mask as a start.


I think it is better to use a mask and not fail, so that we can in the
future use a recent VDUSE application with an older kernel.


It may confuse the userspace unless userspace can do post check after
CREATE_DEV.

And for blk we fail when WCE is set in feature_is_valid():

static bool features_is_valid(u64 features)
{
 if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
 return false;

 /* Now we only support read-only configuration space */
 if (features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE))
 return false;

 return true;
}


Ok, consistency with other devices types is indeed better.

But should I fail if any of the feature advertised by the application is
not listed by the VDUSE driver, or just fail if control queue is being
advertised by the application?

Thanks,
Maxime


Thanks



Why would it be better to fail than negotiating?

Thanks,
Maxime

Re: [PATCH v5 2/4] vduse: Temporarily disable control queue features

[Maxime Coquelin]

Hi Jason,

On 12/13/23 05:52, Jason Wang wrote:

On Tue, Dec 12, 2023 at 9:17 PM Maxime Coquelin
 wrote:


Virtio-net driver control queue implementation is not safe
when used with VDUSE. If the VDUSE application does not
reply to control queue messages, it currently ends up
hanging the kernel thread sending this command.

Some work is on-going to make the control queue
implementation robust with VDUSE. Until it is completed,
let's disable control virtqueue and features that depend on
it.

Signed-off-by: Maxime Coquelin 


I wonder if it's better to fail instead of a mask as a start.


I think it is better to use a mask and not fail, so that we can in the
future use a recent VDUSE application with an older kernel.

Why would it be better to fail than negotiating?

Thanks,
Maxime


Thanks


---
  drivers/vdpa/vdpa_user/vduse_dev.c | 37 ++
  1 file changed, 37 insertions(+)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c 
b/drivers/vdpa/vdpa_user/vduse_dev.c
index 0486ff672408..fe4b5c8203fd 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -28,6 +28,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 

  #include "iova_domain.h"
@@ -46,6 +47,30 @@

  #define IRQ_UNBOUND -1

+#define VDUSE_NET_VALID_FEATURES_MASK   \
+   (BIT_ULL(VIRTIO_NET_F_CSUM) |   \
+BIT_ULL(VIRTIO_NET_F_GUEST_CSUM) | \
+BIT_ULL(VIRTIO_NET_F_MTU) |\
+BIT_ULL(VIRTIO_NET_F_MAC) |\
+BIT_ULL(VIRTIO_NET_F_GUEST_TSO4) | \
+BIT_ULL(VIRTIO_NET_F_GUEST_TSO6) | \
+BIT_ULL(VIRTIO_NET_F_GUEST_ECN) |  \
+BIT_ULL(VIRTIO_NET_F_GUEST_UFO) |  \
+BIT_ULL(VIRTIO_NET_F_HOST_TSO4) |  \
+BIT_ULL(VIRTIO_NET_F_HOST_TSO6) |  \
+BIT_ULL(VIRTIO_NET_F_HOST_ECN) |   \
+BIT_ULL(VIRTIO_NET_F_HOST_UFO) |   \
+BIT_ULL(VIRTIO_NET_F_MRG_RXBUF) |  \
+BIT_ULL(VIRTIO_NET_F_STATUS) | \
+BIT_ULL(VIRTIO_NET_F_HOST_USO) |   \
+BIT_ULL(VIRTIO_F_ANY_LAYOUT) | \
+BIT_ULL(VIRTIO_RING_F_INDIRECT_DESC) | \
+BIT_ULL(VIRTIO_RING_F_EVENT_IDX) |  \
+BIT_ULL(VIRTIO_F_VERSION_1) |  \
+BIT_ULL(VIRTIO_F_ACCESS_PLATFORM) | \
+BIT_ULL(VIRTIO_F_RING_PACKED) |\
+BIT_ULL(VIRTIO_F_IN_ORDER))
+
  struct vduse_virtqueue {
 u16 index;
 u16 num_max;
@@ -1782,6 +1807,16 @@ static struct attribute *vduse_dev_attrs[] = {

  ATTRIBUTE_GROUPS(vduse_dev);

+static void vduse_dev_features_filter(struct vduse_dev_config *config)
+{
+   /*
+* Temporarily filter out virtio-net's control virtqueue and features
+* that depend on it while CVQ is being made more robust for VDUSE.
+*/
+   if (config->device_id == VIRTIO_ID_NET)
+   config->features &= VDUSE_NET_VALID_FEATURES_MASK;
+}
+
  static int vduse_create_dev(struct vduse_dev_config *config,
 void *config_buf, u64 api_version)
  {
@@ -1797,6 +1832,8 @@ static int vduse_create_dev(struct vduse_dev_config 
*config,
 if (!dev)
 goto err;

+   vduse_dev_features_filter(config);
+
 dev->api_version = api_version;
 dev->device_features = config->features;
 dev->device_id = config->device_id;
--
2.43.0

[PATCH v5 4/4] vduse: Add LSM hook to check Virtio device type

[Maxime Coquelin]

This patch introduces a LSM hook for devices creation,
destruction (ioctl()) and opening (open()) operations,
checking the application is allowed to perform these
operations for the Virtio device type.

Signed-off-by: Maxime Coquelin 
---
 MAINTAINERS |  1 +
 drivers/vdpa/vdpa_user/vduse_dev.c  | 13 
 include/linux/lsm_hook_defs.h   |  2 ++
 include/linux/security.h|  6 ++
 include/linux/vduse.h   | 14 +
 security/security.c | 15 ++
 security/selinux/hooks.c| 32 +
 security/selinux/include/classmap.h |  2 ++
 8 files changed, 85 insertions(+)
 create mode 100644 include/linux/vduse.h

diff --git a/MAINTAINERS b/MAINTAINERS
index a0fb0df07b43..4e83b14358d2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23040,6 +23040,7 @@ F:  drivers/net/virtio_net.c
 F: drivers/vdpa/
 F: drivers/virtio/
 F: include/linux/vdpa.h
+F: include/linux/vduse.h
 F: include/linux/virtio*.h
 F: include/linux/vringh.h
 F: include/uapi/linux/virtio_*.h
diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c 
b/drivers/vdpa/vdpa_user/vduse_dev.c
index fa62825be378..59ab7eb62e20 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -8,6 +8,7 @@
  *
  */
 
+#include "linux/security.h"
 #include 
 #include 
 #include 
@@ -30,6 +31,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "iova_domain.h"
 
@@ -1442,6 +1444,10 @@ static int vduse_dev_open(struct inode *inode, struct 
file *file)
if (dev->connected)
goto unlock;
 
+   ret = -EPERM;
+   if (security_vduse_perm_check(VDUSE_PERM_OPEN, dev->device_id))
+   goto unlock;
+
ret = 0;
dev->connected = true;
file->private_data = dev;
@@ -1664,6 +1670,9 @@ static int vduse_destroy_dev(char *name)
if (!dev)
return -EINVAL;
 
+   if (security_vduse_perm_check(VDUSE_PERM_DESTROY, dev->device_id))
+   return -EPERM;
+
mutex_lock(>lock);
if (dev->vdev || dev->connected) {
mutex_unlock(>lock);
@@ -1828,6 +1837,10 @@ static int vduse_create_dev(struct vduse_dev_config 
*config,
int ret;
struct vduse_dev *dev;
 
+   ret = -EPERM;
+   if (security_vduse_perm_check(VDUSE_PERM_CREATE, config->device_id))
+   goto err;
+
ret = -EEXIST;
if (vduse_find_dev(config->name))
goto err;
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index ff217a5ce552..3930ab2ae974 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -419,3 +419,5 @@ LSM_HOOK(int, 0, uring_override_creds, const struct cred 
*new)
 LSM_HOOK(int, 0, uring_sqpoll, void)
 LSM_HOOK(int, 0, uring_cmd, struct io_uring_cmd *ioucmd)
 #endif /* CONFIG_IO_URING */
+
+LSM_HOOK(int, 0, vduse_perm_check, enum vduse_op_perm op_perm, u32 device_id)
diff --git a/include/linux/security.h b/include/linux/security.h
index 1d1df326c881..2a2054172394 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -32,6 +32,7 @@
 #include 
 #include 
 #include 
+#include 
 
 struct linux_binprm;
 struct cred;
@@ -484,6 +485,7 @@ int security_inode_notifysecctx(struct inode *inode, void 
*ctx, u32 ctxlen);
 int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
 int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
 int security_locked_down(enum lockdown_reason what);
+int security_vduse_perm_check(enum vduse_op_perm op_perm, u32 device_id);
 #else /* CONFIG_SECURITY */
 
 static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data)
@@ -1395,6 +1397,10 @@ static inline int security_locked_down(enum 
lockdown_reason what)
 {
return 0;
 }
+static inline int security_vduse_perm_check(enum vduse_op_perm op_perm, u32 
device_id)
+{
+   return 0;
+}
 #endif /* CONFIG_SECURITY */
 
 #if defined(CONFIG_SECURITY) && defined(CONFIG_WATCH_QUEUE)
diff --git a/include/linux/vduse.h b/include/linux/vduse.h
new file mode 100644
index ..7a20dcc43997
--- /dev/null
+++ b/include/linux/vduse.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_VDUSE_H
+#define _LINUX_VDUSE_H
+
+/*
+ * The permission required for a VDUSE device operation.
+ */
+enum vduse_op_perm {
+   VDUSE_PERM_CREATE,
+   VDUSE_PERM_DESTROY,
+   VDUSE_PERM_OPEN,
+};
+
+#endif /* _LINUX_VDUSE_H */
diff --git a/security/security.c b/security/security.c
index dcb3e7014f9b..150abf85f97d 100644
--- a/security/security.c
+++ b/security/security.c
@@ -5337,3 +5337,18 @@ int security_uring_cmd(struct io_uring_cmd *ioucmd)
return call_int_hook(uring_cmd, 0, ioucmd);
 }
 #endif /* CONFIG_IO_URING */
+
+/**
+ * security_vduse_perm_check() - Check if a VDUSE 

[PATCH v5 2/4] vduse: Temporarily disable control queue features

[Maxime Coquelin]

Virtio-net driver control queue implementation is not safe
when used with VDUSE. If the VDUSE application does not
reply to control queue messages, it currently ends up
hanging the kernel thread sending this command.

Some work is on-going to make the control queue
implementation robust with VDUSE. Until it is completed,
let's disable control virtqueue and features that depend on
it.

Signed-off-by: Maxime Coquelin 
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 37 ++
 1 file changed, 37 insertions(+)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c 
b/drivers/vdpa/vdpa_user/vduse_dev.c
index 0486ff672408..fe4b5c8203fd 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "iova_domain.h"
@@ -46,6 +47,30 @@
 
 #define IRQ_UNBOUND -1
 
+#define VDUSE_NET_VALID_FEATURES_MASK   \
+   (BIT_ULL(VIRTIO_NET_F_CSUM) |   \
+BIT_ULL(VIRTIO_NET_F_GUEST_CSUM) | \
+BIT_ULL(VIRTIO_NET_F_MTU) |\
+BIT_ULL(VIRTIO_NET_F_MAC) |\
+BIT_ULL(VIRTIO_NET_F_GUEST_TSO4) | \
+BIT_ULL(VIRTIO_NET_F_GUEST_TSO6) | \
+BIT_ULL(VIRTIO_NET_F_GUEST_ECN) |  \
+BIT_ULL(VIRTIO_NET_F_GUEST_UFO) |  \
+BIT_ULL(VIRTIO_NET_F_HOST_TSO4) |  \
+BIT_ULL(VIRTIO_NET_F_HOST_TSO6) |  \
+BIT_ULL(VIRTIO_NET_F_HOST_ECN) |   \
+BIT_ULL(VIRTIO_NET_F_HOST_UFO) |   \
+BIT_ULL(VIRTIO_NET_F_MRG_RXBUF) |  \
+BIT_ULL(VIRTIO_NET_F_STATUS) | \
+BIT_ULL(VIRTIO_NET_F_HOST_USO) |   \
+BIT_ULL(VIRTIO_F_ANY_LAYOUT) | \
+BIT_ULL(VIRTIO_RING_F_INDIRECT_DESC) | \
+BIT_ULL(VIRTIO_RING_F_EVENT_IDX) |  \
+BIT_ULL(VIRTIO_F_VERSION_1) |  \
+BIT_ULL(VIRTIO_F_ACCESS_PLATFORM) | \
+BIT_ULL(VIRTIO_F_RING_PACKED) |\
+BIT_ULL(VIRTIO_F_IN_ORDER))
+
 struct vduse_virtqueue {
u16 index;
u16 num_max;
@@ -1782,6 +1807,16 @@ static struct attribute *vduse_dev_attrs[] = {
 
 ATTRIBUTE_GROUPS(vduse_dev);
 
+static void vduse_dev_features_filter(struct vduse_dev_config *config)
+{
+   /*
+* Temporarily filter out virtio-net's control virtqueue and features
+* that depend on it while CVQ is being made more robust for VDUSE.
+*/
+   if (config->device_id == VIRTIO_ID_NET)
+   config->features &= VDUSE_NET_VALID_FEATURES_MASK;
+}
+
 static int vduse_create_dev(struct vduse_dev_config *config,
void *config_buf, u64 api_version)
 {
@@ -1797,6 +1832,8 @@ static int vduse_create_dev(struct vduse_dev_config 
*config,
if (!dev)
goto err;
 
+   vduse_dev_features_filter(config);
+
dev->api_version = api_version;
dev->device_features = config->features;
dev->device_id = config->device_id;
-- 
2.43.0

[PATCH v5 3/4] vduse: enable Virtio-net device type

[Maxime Coquelin]

This patch adds Virtio-net device type to the supported
devices types. Initialization fails if the device does
not support VIRTIO_F_VERSION_1 feature, in order to
guarantee the configuration space is read-only.

Acked-by: Jason Wang 
Reviewed-by: Xie Yongji 
Signed-off-by: Maxime Coquelin 
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c 
b/drivers/vdpa/vdpa_user/vduse_dev.c
index fe4b5c8203fd..fa62825be378 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -166,6 +166,7 @@ static struct workqueue_struct *vduse_irq_bound_wq;
 
 static u32 allowed_device_id[] = {
VIRTIO_ID_BLOCK,
+   VIRTIO_ID_NET,
 };
 
 static inline struct vduse_dev *vdpa_to_vduse(struct vdpa_device *vdpa)
@@ -1706,6 +1707,10 @@ static bool features_is_valid(struct vduse_dev_config 
*config)
(config->features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE)))
return false;
 
+   if ((config->device_id == VIRTIO_ID_NET) &&
+   !(config->features & (1ULL << VIRTIO_F_VERSION_1)))
+   return false;
+
return true;
 }
 
@@ -2068,6 +2073,7 @@ static const struct vdpa_mgmtdev_ops vdpa_dev_mgmtdev_ops 
= {
 
 static struct virtio_device_id id_table[] = {
{ VIRTIO_ID_BLOCK, VIRTIO_DEV_ANY_ID },
+   { VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
{ 0 },
 };
 
-- 
2.43.0

[PATCH v5 1/4] vduse: validate block features only with block devices

[Maxime Coquelin]

This patch is preliminary work to enable network device
type support to VDUSE.

As VIRTIO_BLK_F_CONFIG_WCE shares the same value as
VIRTIO_NET_F_HOST_TSO4, we need to restrict its check
to Virtio-blk device type.

Acked-by: Jason Wang 
Reviewed-by: Xie Yongji 
Signed-off-by: Maxime Coquelin 
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c 
b/drivers/vdpa/vdpa_user/vduse_dev.c
index 0ddd4b8abecb..0486ff672408 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -1671,13 +1671,14 @@ static bool device_is_allowed(u32 device_id)
return false;
 }
 
-static bool features_is_valid(u64 features)
+static bool features_is_valid(struct vduse_dev_config *config)
 {
-   if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
+   if (!(config->features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
return false;
 
/* Now we only support read-only configuration space */
-   if (features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE))
+   if ((config->device_id == VIRTIO_ID_BLOCK) &&
+   (config->features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE)))
return false;
 
return true;
@@ -1704,7 +1705,7 @@ static bool vduse_validate_config(struct vduse_dev_config 
*config)
if (!device_is_allowed(config->device_id))
return false;
 
-   if (!features_is_valid(config->features))
+   if (!features_is_valid(config))
return false;
 
return true;
-- 
2.43.0

[PATCH v5 0/4] vduse: add support for networking devices

[Maxime Coquelin]

This small series enables virtio-net device type in VDUSE.
With it, basic operation have been tested, both with
virtio-vdpa and vhost-vdpa using DPDK Vhost library series
adding VDUSE support using split rings layout (merged in
DPDK v23.07-rc1).

Control queue support (and so multiqueue) has also been
tested, but requires a Kernel series from Jason Wang
relaxing control queue polling [1] to function reliably,
so while Jason rework is done, a patch is added to disable
CVQ and features that depend on it (tested also with DPDK
v23.07-rc1).

In this v5, LSM hooks introduced in previous revision are
unified into a single hook that covers below operations:
- VDUSE_CREATE_DEV ioctl on VDUSE control file,
- VDUSE_DESTROY_DEV ioctl on VDUSE control file,
- open() on VDUSE device file.

In combination with the operations permission, a device type
permission has to be associated:
- block: Virtio block device type,
- net: Virtio networking device type.

Changes in v5:
==
- Move control queue disablement patch before Net
  devices enablement (Jason).
- Unify operations LSM hooks into a single hook.
- Rebase on latest master.

Maxime Coquelin (4):
  vduse: validate block features only with block devices
  vduse: Temporarily disable control queue features
  vduse: enable Virtio-net device type
  vduse: Add LSM hook to check Virtio device type

 MAINTAINERS |  1 +
 drivers/vdpa/vdpa_user/vduse_dev.c  | 65 +++--
 include/linux/lsm_hook_defs.h   |  2 +
 include/linux/security.h|  6 +++
 include/linux/vduse.h   | 14 +++
 security/security.c | 15 +++
 security/selinux/hooks.c| 32 ++
 security/selinux/include/classmap.h |  2 +
 8 files changed, 133 insertions(+), 4 deletions(-)
 create mode 100644 include/linux/vduse.h

-- 
2.43.0

Re: [PATCH v4 4/4] vduse: Add LSM hooks to check Virtio device type

[Maxime Coquelin]

On 12/8/23 13:26, Michael S. Tsirkin wrote:

On Fri, Dec 08, 2023 at 01:23:00PM +0100, Maxime Coquelin wrote:



On 12/8/23 12:05, Michael S. Tsirkin wrote:

On Fri, Dec 08, 2023 at 12:01:15PM +0100, Maxime Coquelin wrote:

Hello Paul,

On 11/8/23 03:31, Paul Moore wrote:

On Oct 20, 2023 "Michael S. Tsirkin"  wrote:


This patch introduces LSM hooks for devices creation,
destruction and opening operations, checking the
application is allowed to perform these operations for
the Virtio device type.

Signed-off-by: Maxime Coquelin 
---
drivers/vdpa/vdpa_user/vduse_dev.c  | 12 +++
include/linux/lsm_hook_defs.h   |  4 +++
include/linux/security.h| 15 
security/security.c | 42 ++
security/selinux/hooks.c| 55 +
security/selinux/include/classmap.h |  2 ++
6 files changed, 130 insertions(+)


My apologies for the late reply, I've been trying to work my way through
the review backlog but it has been taking longer than expected; comments
below ...


No worries, I have also been busy these days.


diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 2aa0e219d721..65d9262a37f7 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -21,6 +21,7 @@
 *  Copyright (C) 2016 Mellanox Technologies
 */
+#include "av_permissions.h"
#include 
#include 
#include 
@@ -92,6 +93,7 @@
#include 
#include 
#include 
+#include 
#include "avc.h"
#include "objsec.h"
@@ -6950,6 +6952,56 @@ static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
}
#endif /* CONFIG_IO_URING */
+static int vduse_check_device_type(u32 sid, u32 device_id)
+{
+   u32 requested;
+
+   if (device_id == VIRTIO_ID_NET)
+   requested = VDUSE__NET;
+   else if (device_id == VIRTIO_ID_BLOCK)
+   requested = VDUSE__BLOCK;
+   else
+   return -EINVAL;
+
+   return avc_has_perm(sid, sid, SECCLASS_VDUSE, requested, NULL);
+}
+
+static int selinux_vduse_dev_create(u32 device_id)
+{
+   u32 sid = current_sid();
+   int ret;
+
+   ret = avc_has_perm(sid, sid, SECCLASS_VDUSE, VDUSE__DEVCREATE, NULL);
+   if (ret)
+   return ret;
+
+   return vduse_check_device_type(sid, device_id);
+}


I see there has been some discussion about the need for a dedicated
create hook as opposed to using the existing ioctl controls.  I think
one important point that has been missing from the discussion is the
idea of labeling the newly created device.  Unfortunately prior to a
few minutes ago I hadn't ever looked at VDUSE so please correct me if
I get some things wrong :)

   From what I can see userspace creates a new VDUSE device with
ioctl(VDUSE_CREATE_DEV), which trigger the creation of a new
/dev/vduse/XXX device which will be labeled according to the udev
and SELinux configuration, likely with a generic udev label.  My
question is if we want to be able to uniquely label each VDUSE
device based on the process that initiates the device creation
with the call to ioctl()?  If that is the case, we would need a
create hook not only to control the creation of the device, but to
record the triggering process' label in the new device; this label
would then be used in subsequent VDUSE open and destroy operations.
The normal device file I/O operations would still be subject to the
standard SELinux file I/O permissions using the device file label
assigned by systemd/udev when the device was created.


I don't think we need a unique label for VDUSE devices, but maybe
Michael thinks otherwise?


I don't know.
All this is consumed by libvirt, you need to ask these guys.


I think it is not consumed by libvirt, at least not in the usecases I
have in mind. For networking devices, it will be consumed by OVS.

Maxime


OK, ovs then :)



Adding Aaron, our go-to person for SELinux-related topics for OVS, but I 
think we don't need to do relabeling for VDUSE chardevs.


Aaron, do you confirm?

Maxime

Re: [PATCH v4 4/4] vduse: Add LSM hooks to check Virtio device type

[Maxime Coquelin]

On 12/8/23 12:05, Michael S. Tsirkin wrote:

On Fri, Dec 08, 2023 at 12:01:15PM +0100, Maxime Coquelin wrote:

Hello Paul,

On 11/8/23 03:31, Paul Moore wrote:

On Oct 20, 2023 "Michael S. Tsirkin"  wrote:


This patch introduces LSM hooks for devices creation,
destruction and opening operations, checking the
application is allowed to perform these operations for
the Virtio device type.

Signed-off-by: Maxime Coquelin 
---
   drivers/vdpa/vdpa_user/vduse_dev.c  | 12 +++
   include/linux/lsm_hook_defs.h   |  4 +++
   include/linux/security.h| 15 
   security/security.c | 42 ++
   security/selinux/hooks.c| 55 +
   security/selinux/include/classmap.h |  2 ++
   6 files changed, 130 insertions(+)


My apologies for the late reply, I've been trying to work my way through
the review backlog but it has been taking longer than expected; comments
below ...


No worries, I have also been busy these days.


diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 2aa0e219d721..65d9262a37f7 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -21,6 +21,7 @@
*  Copyright (C) 2016 Mellanox Technologies
*/
+#include "av_permissions.h"
   #include 
   #include 
   #include 
@@ -92,6 +93,7 @@
   #include 
   #include 
   #include 
+#include 
   #include "avc.h"
   #include "objsec.h"
@@ -6950,6 +6952,56 @@ static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
   }
   #endif /* CONFIG_IO_URING */
+static int vduse_check_device_type(u32 sid, u32 device_id)
+{
+   u32 requested;
+
+   if (device_id == VIRTIO_ID_NET)
+   requested = VDUSE__NET;
+   else if (device_id == VIRTIO_ID_BLOCK)
+   requested = VDUSE__BLOCK;
+   else
+   return -EINVAL;
+
+   return avc_has_perm(sid, sid, SECCLASS_VDUSE, requested, NULL);
+}
+
+static int selinux_vduse_dev_create(u32 device_id)
+{
+   u32 sid = current_sid();
+   int ret;
+
+   ret = avc_has_perm(sid, sid, SECCLASS_VDUSE, VDUSE__DEVCREATE, NULL);
+   if (ret)
+   return ret;
+
+   return vduse_check_device_type(sid, device_id);
+}


I see there has been some discussion about the need for a dedicated
create hook as opposed to using the existing ioctl controls.  I think
one important point that has been missing from the discussion is the
idea of labeling the newly created device.  Unfortunately prior to a
few minutes ago I hadn't ever looked at VDUSE so please correct me if
I get some things wrong :)

  From what I can see userspace creates a new VDUSE device with
ioctl(VDUSE_CREATE_DEV), which trigger the creation of a new
/dev/vduse/XXX device which will be labeled according to the udev
and SELinux configuration, likely with a generic udev label.  My
question is if we want to be able to uniquely label each VDUSE
device based on the process that initiates the device creation
with the call to ioctl()?  If that is the case, we would need a
create hook not only to control the creation of the device, but to
record the triggering process' label in the new device; this label
would then be used in subsequent VDUSE open and destroy operations.
The normal device file I/O operations would still be subject to the
standard SELinux file I/O permissions using the device file label
assigned by systemd/udev when the device was created.


I don't think we need a unique label for VDUSE devices, but maybe
Michael thinks otherwise?


I don't know.
All this is consumed by libvirt, you need to ask these guys.


I think it is not consumed by libvirt, at least not in the usecases I
have in mind. For networking devices, it will be consumed by OVS.

Maxime

Re: [PATCH v4 4/4] vduse: Add LSM hooks to check Virtio device type

[Maxime Coquelin]

Hello Paul,

On 11/8/23 03:31, Paul Moore wrote:

On Oct 20, 2023 "Michael S. Tsirkin"  wrote:


This patch introduces LSM hooks for devices creation,
destruction and opening operations, checking the
application is allowed to perform these operations for
the Virtio device type.

Signed-off-by: Maxime Coquelin 
---
  drivers/vdpa/vdpa_user/vduse_dev.c  | 12 +++
  include/linux/lsm_hook_defs.h   |  4 +++
  include/linux/security.h| 15 
  security/security.c | 42 ++
  security/selinux/hooks.c| 55 +
  security/selinux/include/classmap.h |  2 ++
  6 files changed, 130 insertions(+)


My apologies for the late reply, I've been trying to work my way through
the review backlog but it has been taking longer than expected; comments
below ...


No worries, I have also been busy these days.


diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 2aa0e219d721..65d9262a37f7 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -21,6 +21,7 @@
   *  Copyright (C) 2016 Mellanox Technologies
   */
  
+#include "av_permissions.h"

  #include 
  #include 
  #include 
@@ -92,6 +93,7 @@
  #include 
  #include 
  #include 
+#include 
  
  #include "avc.h"

  #include "objsec.h"
@@ -6950,6 +6952,56 @@ static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
  }
  #endif /* CONFIG_IO_URING */
  
+static int vduse_check_device_type(u32 sid, u32 device_id)

+{
+   u32 requested;
+
+   if (device_id == VIRTIO_ID_NET)
+   requested = VDUSE__NET;
+   else if (device_id == VIRTIO_ID_BLOCK)
+   requested = VDUSE__BLOCK;
+   else
+   return -EINVAL;
+
+   return avc_has_perm(sid, sid, SECCLASS_VDUSE, requested, NULL);
+}
+
+static int selinux_vduse_dev_create(u32 device_id)
+{
+   u32 sid = current_sid();
+   int ret;
+
+   ret = avc_has_perm(sid, sid, SECCLASS_VDUSE, VDUSE__DEVCREATE, NULL);
+   if (ret)
+   return ret;
+
+   return vduse_check_device_type(sid, device_id);
+}


I see there has been some discussion about the need for a dedicated
create hook as opposed to using the existing ioctl controls.  I think
one important point that has been missing from the discussion is the
idea of labeling the newly created device.  Unfortunately prior to a
few minutes ago I hadn't ever looked at VDUSE so please correct me if
I get some things wrong :)

 From what I can see userspace creates a new VDUSE device with
ioctl(VDUSE_CREATE_DEV), which trigger the creation of a new
/dev/vduse/XXX device which will be labeled according to the udev
and SELinux configuration, likely with a generic udev label.  My
question is if we want to be able to uniquely label each VDUSE
device based on the process that initiates the device creation
with the call to ioctl()?  If that is the case, we would need a
create hook not only to control the creation of the device, but to
record the triggering process' label in the new device; this label
would then be used in subsequent VDUSE open and destroy operations.
The normal device file I/O operations would still be subject to the
standard SELinux file I/O permissions using the device file label
assigned by systemd/udev when the device was created.


I don't think we need a unique label for VDUSE devices, but maybe
Michael thinks otherwise?




+static int selinux_vduse_dev_destroy(u32 device_id)
+{
+   u32 sid = current_sid();
+   int ret;
+
+   ret = avc_has_perm(sid, sid, SECCLASS_VDUSE, VDUSE__DEVDESTROY, NULL);
+   if (ret)
+   return ret;
+
+   return vduse_check_device_type(sid, device_id);
+}
+
+static int selinux_vduse_dev_open(u32 device_id)
+{
+   u32 sid = current_sid();
+   int ret;
+
+   ret = avc_has_perm(sid, sid, SECCLASS_VDUSE, VDUSE__DEVOPEN, NULL);
+   if (ret)
+   return ret;
+
+   return vduse_check_device_type(sid, device_id);
+}
+
  /*
   * IMPORTANT NOTE: When adding new hooks, please be careful to keep this 
order:
   * 1. any hooks that don't belong to (2.) or (3.) below,
@@ -7243,6 +7295,9 @@ static struct security_hook_list selinux_hooks[] 
__ro_after_init = {
  #ifdef CONFIG_PERF_EVENTS
LSM_HOOK_INIT(perf_event_alloc, selinux_perf_event_alloc),
  #endif
+   LSM_HOOK_INIT(vduse_dev_create, selinux_vduse_dev_create),
+   LSM_HOOK_INIT(vduse_dev_destroy, selinux_vduse_dev_destroy),
+   LSM_HOOK_INIT(vduse_dev_open, selinux_vduse_dev_open),
  };
  
  static __init int selinux_init(void)

diff --git a/security/selinux/include/classmap.h 
b/security/selinux/include/classmap.h
index a3c380775d41..d3dc37fb03d4 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -256,6 +256,8 @@ const struct security_class_mapping secclass_map[] = {
  { "override_creds", "sqpoll", "cmd&q

Re: [PATCH 1/2] vdpa: ifcvf: return err when fail to request config irq

[Maxime Coquelin]

Hi,

On 7/23/20 11:12 AM, Jason Wang wrote:
> We ignore the err of requesting config interrupt, fix this.
> 
> Fixes: e7991f376a4d ("ifcvf: implement config interrupt in IFCVF")
> Cc: Zhu Lingshan 
> Signed-off-by: Jason Wang 
> ---
>  drivers/vdpa/ifcvf/ifcvf_main.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index f5a60c14b979..ae7110955a44 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -76,6 +76,10 @@ static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
>   ret = devm_request_irq(>dev, irq,
>  ifcvf_config_changed, 0,
>  vf->config_msix_name, vf);
> + if (ret) {
> + IFCVF_ERR(pdev, "Failed to request config irq\n");
> + return ret;
> + }
>  
>   for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
>   snprintf(vf->vring[i].msix_name, 256, "ifcvf[%s]-%d\n",
> 

This series fixes below Kernel BUG I faced while doing some experiments:

[ 1398.695362] kernel BUG at drivers/pci/msi.c:375!
[ 1398.700561] invalid opcode:  [#1] SMP PTI
[ 1398.705423] CPU: 0 PID: 25110 Comm: dpdk-testpmd Not tainted
5.8.0-amorenoz-vdpa+ #2
[ 1398.714063] Hardware name: Dell Inc. PowerEdge R730/072T6D, BIOS
2.4.3 01/17/2017
[ 1398.722415] RIP: 0010:free_msi_irqs+0x189/0x1c0
[ 1398.727470] Code: 14 85 c0 0f 84 cc fe ff ff 31 ed eb 0f 83 c5 01 39
6b 14 0f 86 bc fe ff ff 8b 7b 10 01 ef e8 7e 94 b9 ff 48 83 78 70 00d
[ 1398.748426] RSP: 0018:b48ac5dd3db8 EFLAGS: 00010286
[ 1398.754257] RAX: 9ab298b85400 RBX: 9ab285d97100 RCX:

[ 1398.762219] RDX:  RSI: 0073 RDI:
ac65e8a0
[ 1398.770182] RBP:  R08: 9ab298b85400 R09:
9ab74a8c3d98
[ 1398.778145] R10: 9ab74a8c3f58 R11:  R12:
9ab719fd82e0
[ 1398.786107] R13: 9ab719fd8000 R14: 9ab719fd8000 R15:
9ab719fd80b0
[ 1398.794069] FS:  7efc5dea9000() GS:9ab75fc0()
knlGS:
[ 1398.803099] CS:  0010 DS:  ES:  CR0: 80050033
[ 1398.809508] CR2: 00c79ff8 CR3: 00038283e005 CR4:
003606f0
[ 1398.817471] DR0:  DR1:  DR2:

[ 1398.825434] DR3:  DR6: fffe0ff0 DR7:
0400
[ 1398.833394] Call Trace:
[ 1398.836127]  pci_disable_msix+0xf7/0x120
[ 1398.840504]  pci_free_irq_vectors+0xe/0x20
[ 1398.845074]  ifcvf_vdpa_set_status+0xda/0x301
[ 1398.849938]  vhost_vdpa_unlocked_ioctl+0x61d/0x790
[ 1398.855277]  ? vhost_vdpa_process_iotlb_msg+0x2f0/0x2f0
[ 1398.861109]  ksys_ioctl+0x87/0xc0
[ 1398.864808]  __x64_sys_ioctl+0x16/0x20
[ 1398.868992]  do_syscall_64+0x52/0x90
[ 1398.872982]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 1398.878610] RIP: 0033:0x7efc5df9ff9b
[ 1398.882598] Code: 0f 1e fa 48 8b 05 ed ce 0c 00 64 c7 00 26 00 00 00
48 c7 c0 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa b8 10 00 00 008
[ 1398.903551] RSP: 002b:7ffd0948e378 EFLAGS: 0283 ORIG_RAX:
0010
[ 1398.911998] RAX: ffda RBX:  RCX:
7efc5df9ff9b
[ 1398.919960] RDX: 7ffd0948e3d4 RSI: 4001af72 RDI:
002c
[ 1398.927921] RBP: 7ffd0948e3c0 R08: 02651bf8 R09:

[ 1398.935883] R10: 7ffd0948e417 R11: 0283 R12:
00408950
[ 1398.943845] R13: 7ffd0948e6a0 R14:  R15:

[ 1398.951809] Modules linked in: vxlan ip6_udp_tunnel udp_tunnel
ip_vs_sh ip_vs_wrr ip_vs_rr ip_vs xt_comment xt_mark nf_tables xt_nat vetht
[ 1398.951847]  ghash_clmulni_intel iTCO_vendor_support mlx5_core dcdbas
rapl intel_cstate intel_uncore ipmi_ssif pcspkr mxm_wmi mlxfw virtii

Tested-by: Maxime Coquelin 

Thanks,
Maxime

Re: [PATCH] watchdog: Respect watchdog cpumask on CPU hotplug

[Maxime Coquelin]

On 3/27/19 8:10 PM, Thomas Gleixner wrote:

On Wed, 27 Mar 2019, Oleg Nesterov wrote:

On 03/26, Thomas Gleixner wrote:


The rework of the watchdog core to use cpu_stop_work broke the watchdog
cpumask on CPU hotplug.

The watchdog_enable/disable() functions are now called unconditionally from
the hotplug callback, i.e. even on CPUs which are not in the watchdog
cpumask.

Only invoke them when the plugged CPU is in the watchdog cpumask.


IIUC without this fix an NMI watchdog can too be enabled at boot time even
if the initial watchdog_cpumask = housekeeping_cpumask(HK_FLAG_TIMER) doesn't
include the plugged CPU.


Yes.


And after that writing 0 to /proc/sys/kernel/nmi_watchdog clears
NMI_WATCHDOG_ENABLED but this can't disable NMI watchdog's outside of
watchdog_allowed_mask.


Correct


So may be this can explain the problem reported by Maxime ?
See 
https://lore.kernel.org/lkml/b99c5a25-a5fe-18dd-2f1d-bdd6834f0...@redhat.com/


That looks so.


I had a trial with your patch, and I can confirm it fixes my issue:

Tested-by: Maxime Coquelin 

Thanks,
Maxime


Thanks,

tglx

[Regression]: NMI watchdog regression from v4.19 onwards

[Maxime Coquelin]

Hi Peter, Oleg,

NMI watchdog fires systematically on my machine with recent Kernels, 
whereas the NMI watch is supposed to be disabled:


# cat /proc/sys/kernel/watchdog
0
# cat /proc/sys/kernel/nmi_watchdog
0
#
[   53.765648] NMI watchdog: Watchdog detected hard LOCKUP on cpu 7
[   53.765648] Modules linked in: nft_chain_route_ipv4 xt_CHECKSUM 
nft_chain_nat_ipv4 ipt_MASQUERADE xt_conntrack ipt_REJECT nf_reject_ipv4 
nft_counter nft_cd

[   53.765661] CPU: 7 PID: 0 Comm: swapper/7 Not tainted 5.0.0 #22
[   53.765661] Hardware name: Dell Inc. PowerEdge R730/072T6D, BIOS 
2.4.3 01/17/2017

[   53.765661] RIP: 0010:poll_idle+0xa0/0x102
[   53.765662] Code: 54 48 69 db e8 03 00 00 eb 1b f3 90 83 e8 01 75 19 
65 8b 3d e2 a4 5b 6e e8 fd 9f 8a ff 48 29 e8 48 39 d8 77 60 b8 c9 00 00 
00 <65> 48 8b5

[   53.765662] RSP: :a77ac01f3e70 EFLAGS: 0206
[   53.765663] RAX: 000a RBX: 07d0 RCX: 
001f
[   53.765663] RDX:  RSI: 28000347 RDI: 
f386de8dd002
[   53.765663] RBP: 000c84ad17fc R08: 0004 R09: 
00021540
[   53.765664] R10: 28124ceb5144 R11: 0010 R12: 
9905dfbea648
[   53.765664] R13:  R14:  R15: 

[   53.765664] FS:  () GS:9905dfbc() 
knlGS:

[   53.765665] CS:  0010 DS:  ES:  CR0: 80050033
[   53.765665] CR2:  CR3: 00037f60e001 CR4: 
003606e0
[   53.765665] DR0:  DR1:  DR2: 

[   53.765665] DR3:  DR6: fffe0ff0 DR7: 
0400

[   53.765666] Call Trace:
[   53.765666]  cpuidle_enter_state+0x73/0x440
[   53.765666]  do_idle+0x1f1/0x230
[   53.765666]  cpu_startup_entry+0x19/0x20
[   53.765667]  start_secondary+0x195/0x1e0
[   53.765667]  secondary_startup_64+0xb6/0xc0
[   53.765667] Kernel panic - not syncing: Hard LOCKUP
[   53.765667] CPU: 7 PID: 0 Comm: swapper/7 Not tainted 5.0.0 #22
[   53.765668] Hardware name: Dell Inc. PowerEdge R730/072T6D, BIOS 
2.4.3 01/17/2017

[   53.765668] Call Trace:
[   53.765668]  
[   53.765668]  dump_stack+0x46/0x60
[   53.765669]  panic+0xfb/0x29b
[   53.765669]  ? startup_64+0x1/0x30
[   53.765669]  ? startup_64+0x30/0x30
[   53.765669]  nmi_panic.cold.9+0xc/0xc
[   53.765670]  watchdog_overflow_callback.cold.7+0x5c/0x70
[   53.765670]  __perf_event_overflow+0x52/0xe0
[   53.765670]  handle_pmi_common+0x1b3/0x210
[   53.765670]  ? set_pte_vaddr_p4d+0x4a/0x60
[   53.765671]  ? ghes_copy_tofrom_phys+0xc0/0x170
[   53.765671]  intel_pmu_handle_irq+0xc9/0x1c0
[   53.765671]  perf_event_nmi_handler+0x2d/0x50
[   53.765671]  nmi_handle+0x63/0x110
[   53.765672]  default_do_nmi+0x4e/0x100
[   53.765672]  do_nmi+0x100/0x160
[   53.765672]  end_repeat_nmi+0x16/0x50
[   53.765672] RIP: 0010:poll_idle+0xa0/0x102
[   53.765673] Code: 54 48 69 db e8 03 00 00 eb 1b f3 90 83 e8 01 75 19 
65 8b 3d e2 a4 5b 6e e8 fd 9f 8a ff 48 29 e8 48 39 d8 77 60 b8 c9 00 00 
00 <65> 48 8b5

[   53.765673] RSP: :a77ac01f3e70 EFLAGS: 0206
[   53.765674] RAX: 000a RBX: 07d0 RCX: 
001f
[   53.765674] RDX:  RSI: 28000347 RDI: 
f386de8dd002
[   53.765674] RBP: 000c84ad17fc R08: 0004 R09: 
00021540
[   53.765675] R10: 28124ceb5144 R11: 0010 R12: 
9905dfbea648
[   53.765675] R13:  R14:  R15: 


[   53.765675]  ? poll_idle+0xa0/0x102
[   53.765675]  ? poll_idle+0xa0/0x102
[   53.765676]  
[   53.765676]  cpuidle_enter_state+0x73/0x440
[   53.765676]  do_idle+0x1f1/0x230
[   53.765676]  cpu_startup_entry+0x19/0x20
[   53.765677]  start_secondary+0x195/0x1e0
[   53.765677]  secondary_startup_64+0xb6/0xc0
[   54.804636] NMI watchdog: Watchdog detected hard LOCKUP on cpu 6
[   54.804637] Modules linked in: nft_chain_route_ipv4 xt_CHECKSUM 
nft_chain_nat_ipv4 ipt_MASQUERADE xt_conntrack ipt_REJECT nf_reject_ipv4 
nft_counter nft_cd

[   54.804649] CPU: 6 PID: 0 Comm: swapper/6 Not tainted 5.0.0 #22
[   54.804649] Hardware name: Dell Inc. PowerEdge R730/072T6D, BIOS 
2.4.3 01/17/2017

[   54.804649] RIP: 0010:poll_idle+0xa0/0x102
[   54.804650] Code: 54 48 69 db e8 03 00 00 eb 1b f3 90 83 e8 01 75 19 
65 8b 3d e2 a4 5b 6e e8 fd 9f 8a ff 48 29 e8 48 39 d8 77 60 b8 c9 00 00 
00 <65> 48 8b5

[   54.804650] RSP: :a77ac01ebe70 EFLAGS: 0202
[   54.804650] RAX: 0046 RBX: 07d0 RCX: 
001f
[   54.804651] RDX:  RSI: 28000347 RDI: 
f386de8dd002
[   54.804651] RBP: 000c84aebd9f R08: 0002 R09: 
00021540
[   54.804651] R10: 28124cf09684 R11: 0032 R12: 
9905dfbaa648
[   54.804652] R13:  R14:  R15: 

[   54.804652] FS:  () GS:9905dfb8() 

Re: [PATCH] MAINTAINERS: update entries for ARM/STM32

[Maxime Coquelin]

2018-02-26 14:03 GMT+01:00 Alexandre Torgue <alexandre.tor...@st.com>:
> Changes old git repository to the maintained one and adds more patterns.
>
> Signed-off-by: Alexandre Torgue <alexandre.tor...@st.com>
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3bdc260..802b984 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1999,8 +1999,10 @@ M:   Maxime Coquelin <mcoquelin.st...@gmail.com>
>  M: Alexandre Torgue <alexandre.tor...@st.com>
>  L: linux-arm-ker...@lists.infradead.org (moderated for non-subscribers)
>  S: Maintained
> -T: git git://git.kernel.org/pub/scm/linux/kernel/git/mcoquelin/stm32.git
> +T: git git://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git 
> stm32-next
>  N: stm32
> +F: arch/arm/boot/dts/stm32*
> +F: arch/arm/mach-stm32/
>  F: drivers/clocksource/armv7m_systick.c
>
>  ARM/TANGO ARCHITECTURE
> --
> 2.7.4
>

Acked-by: Maxime Coquelin <mcoquelin.st...@gmail.com>

Thanks,
Maxime

Re: [PATCH] MAINTAINERS: update entries for ARM/STM32

[Maxime Coquelin]

2018-02-26 14:03 GMT+01:00 Alexandre Torgue :
> Changes old git repository to the maintained one and adds more patterns.
>
> Signed-off-by: Alexandre Torgue 
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3bdc260..802b984 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1999,8 +1999,10 @@ M:   Maxime Coquelin 
>  M: Alexandre Torgue 
>  L: linux-arm-ker...@lists.infradead.org (moderated for non-subscribers)
>  S: Maintained
> -T: git git://git.kernel.org/pub/scm/linux/kernel/git/mcoquelin/stm32.git
> +T: git git://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git 
> stm32-next
>  N: stm32
> +F: arch/arm/boot/dts/stm32*
> +F: arch/arm/mach-stm32/
>  F: drivers/clocksource/armv7m_systick.c
>
>  ARM/TANGO ARCHITECTURE
> --
> 2.7.4
>

Acked-by: Maxime Coquelin 

Thanks,
Maxime

Re: [PATCH v2] irqchip: stm32: Fix copyright

[Maxime Coquelin]

2017-11-30 9:45 GMT+01:00 Benjamin Gaignard <benjamin.gaign...@linaro.org>:
> Uniformize STMicroelectronics copyrights header
> Add SPDX identifier
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaign...@st.com>
> Acked-by: Alexandre TORGUE <alexandre.tor...@st.com>
> CC: Maxime Coquelin <mcoquelin.st...@gmail.com>
> ---
>  drivers/irqchip/irq-stm32-exti.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-stm32-exti.c 
> b/drivers/irqchip/irq-stm32-exti.c
> index 31ab0dee2ce7..36f0fbe36c35 100644
> --- a/drivers/irqchip/irq-stm32-exti.c
> +++ b/drivers/irqchip/irq-stm32-exti.c
> @@ -1,7 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * Copyright (C) Maxime Coquelin 2015
> + * Copyright (C) STMicroelectronics 2017
>   * Author:  Maxime Coquelin <mcoquelin.st...@gmail.com>
> - * License terms:  GNU General Public License (GPL), version 2
>   */
>
>  #include 
> --
> 2.15.0
>

Acked-by: Maxime Coquelin <mcoquelin.st...@gmail.com>

Maxime

Re: [PATCH v2] irqchip: stm32: Fix copyright

[Maxime Coquelin]

2017-11-30 9:45 GMT+01:00 Benjamin Gaignard :
> Uniformize STMicroelectronics copyrights header
> Add SPDX identifier
>
> Signed-off-by: Benjamin Gaignard 
> Acked-by: Alexandre TORGUE 
> CC: Maxime Coquelin 
> ---
>  drivers/irqchip/irq-stm32-exti.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-stm32-exti.c 
> b/drivers/irqchip/irq-stm32-exti.c
> index 31ab0dee2ce7..36f0fbe36c35 100644
> --- a/drivers/irqchip/irq-stm32-exti.c
> +++ b/drivers/irqchip/irq-stm32-exti.c
> @@ -1,7 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * Copyright (C) Maxime Coquelin 2015
> + * Copyright (C) STMicroelectronics 2017
>   * Author:  Maxime Coquelin 
> - * License terms:  GNU General Public License (GPL), version 2
>   */
>
>  #include 
> --
> 2.15.0
>

Acked-by: Maxime Coquelin 

Maxime

Re: [PATCH v2] pinctrl: stm32: Fix copyright

[Maxime Coquelin]

2017-11-30 9:46 GMT+01:00 Benjamin Gaignard <benjamin.gaign...@linaro.org>:
> Uniformize STMicroelectronics copyrights header
> Add SPDX identifier
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaign...@st.com>
> Acked-by: Alexandre TORGUE <alexandre.tor...@st.com>
> CC: Maxime Coquelin <mcoquelin.st...@gmail.com>
> ---
>  drivers/pinctrl/stm32/pinctrl-stm32.c | 3 ++-
>  drivers/pinctrl/stm32/pinctrl-stm32.h | 3 ++-
>  drivers/pinctrl/stm32/pinctrl-stm32f429.c | 3 ++-
>  drivers/pinctrl/stm32/pinctrl-stm32f469.c | 6 +++---
>  drivers/pinctrl/stm32/pinctrl-stm32f746.c | 3 ++-
>  drivers/pinctrl/stm32/pinctrl-stm32h743.c | 6 +++---
>  6 files changed, 14 insertions(+), 10 deletions(-)


Acked-by: Maxime Coquelin <mcoquelin.st...@gmail.com>

Maxime

Re: [PATCH v2] pinctrl: stm32: Fix copyright

[Maxime Coquelin]

2017-11-30 9:46 GMT+01:00 Benjamin Gaignard :
> Uniformize STMicroelectronics copyrights header
> Add SPDX identifier
>
> Signed-off-by: Benjamin Gaignard 
> Acked-by: Alexandre TORGUE 
> CC: Maxime Coquelin 
> ---
>  drivers/pinctrl/stm32/pinctrl-stm32.c | 3 ++-
>  drivers/pinctrl/stm32/pinctrl-stm32.h | 3 ++-
>  drivers/pinctrl/stm32/pinctrl-stm32f429.c | 3 ++-
>  drivers/pinctrl/stm32/pinctrl-stm32f469.c | 6 +++---
>  drivers/pinctrl/stm32/pinctrl-stm32f746.c | 3 ++-
>  drivers/pinctrl/stm32/pinctrl-stm32h743.c | 6 +++---
>  6 files changed, 14 insertions(+), 10 deletions(-)


Acked-by: Maxime Coquelin 

Maxime

Re: [PATCH v4] arch: arm: mach-stm32: Fix copyright

[Maxime Coquelin]

2017-11-30 9:49 GMT+01:00 Benjamin Gaignard <benjamin.gaign...@linaro.org>:
> Uniformize STMicroelectronics copyrights header
> Add SPDX identifier
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaign...@st.com>
> CC: Maxime Coquelin <mcoquelin.st...@gmail.com>
> ---
>  arch/arm/mach-stm32/board-dt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-stm32/board-dt.c b/arch/arm/mach-stm32/board-dt.c
> index e918686e4191..548a19f97129 100644
> --- a/arch/arm/mach-stm32/board-dt.c
> +++ b/arch/arm/mach-stm32/board-dt.c
> @@ -1,7 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * Copyright (C) Maxime Coquelin 2015
> + * Copyright (C) STMicroelectronics 2017
>   * Author:  Maxime Coquelin <mcoquelin.st...@gmail.com>
> - * License terms:  GNU General Public License (GPL), version 2
>   */
>
>  #include 
> --
> 2.15.0
>

Acked-by: Maxime Coquelin <mcoquelin.st...@gmail.com>

Maxime

Re: [PATCH v4] arch: arm: mach-stm32: Fix copyright

[Maxime Coquelin]

2017-11-30 9:49 GMT+01:00 Benjamin Gaignard :
> Uniformize STMicroelectronics copyrights header
> Add SPDX identifier
>
> Signed-off-by: Benjamin Gaignard 
> CC: Maxime Coquelin 
> ---
>  arch/arm/mach-stm32/board-dt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-stm32/board-dt.c b/arch/arm/mach-stm32/board-dt.c
> index e918686e4191..548a19f97129 100644
> --- a/arch/arm/mach-stm32/board-dt.c
> +++ b/arch/arm/mach-stm32/board-dt.c
> @@ -1,7 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * Copyright (C) Maxime Coquelin 2015
> + * Copyright (C) STMicroelectronics 2017
>   * Author:  Maxime Coquelin 
> - * License terms:  GNU General Public License (GPL), version 2
>   */
>
>  #include 
> --
> 2.15.0
>

Acked-by: Maxime Coquelin 

Maxime

Re: [PATCH v6 2/4] drivers: irqchip: Add STM32 external interrupts support

[Maxime Coquelin]

2016-09-21 9:50 GMT+02:00 Thomas Gleixner <t...@linutronix.de>:
> On Wed, 21 Sep 2016, Alexandre Torgue wrote:
>
>> Hi Thomas,
>>
>> On 09/20/2016 10:16 PM, Thomas Gleixner wrote:
>> > Alexandre,
>> >
>> > On Tue, 20 Sep 2016, Alexandre TORGUE wrote:
>> >
>> > > The STM32 external interrupt controller consists of edge detectors that
>> > > generate interrupts requests or wake-up events.
>> > >
>> > > Each line can be independently configured as interrupt or wake-up source,
>> > > and triggers either on rising, falling or both edges. Each line can also
>> > > be masked independently.
>> > >
>> > > Signed-off-by: Maxime Coquelin <mcoquelin.st...@gmail.com>
>> > > Signed-off-by: Alexandre TORGUE <alexandre.tor...@st.com>
>> >
>> > That all looks very reasonable now. The only remaining question is your SOB
>> > chain. Who is the author of these patches? You or Maxime? If it's Maxime,
>> > then the changelog misses a From: tag. If it's you then Maximes SOB is
>> > bogus.
>>
>> Actually Maxime wrote the main part of this driver and sent version 1 and 2 
>> of
>> the series. After Linus W. reviews, rework was required to use hierarchical
>> domain. According to Maxime, I coded the rework (adaptation to hierarchical
>> domain) and sent other version of the series.
>
> So I replace Maximes SOB with Originally-by: Does that apply to all four
> patches?
Yes, that's fine.
Alex did a lot of rework on this series, he deserves the SoB.

Thanks,
Maxime

Re: [PATCH v6 2/4] drivers: irqchip: Add STM32 external interrupts support

[Maxime Coquelin]

2016-09-21 9:50 GMT+02:00 Thomas Gleixner :
> On Wed, 21 Sep 2016, Alexandre Torgue wrote:
>
>> Hi Thomas,
>>
>> On 09/20/2016 10:16 PM, Thomas Gleixner wrote:
>> > Alexandre,
>> >
>> > On Tue, 20 Sep 2016, Alexandre TORGUE wrote:
>> >
>> > > The STM32 external interrupt controller consists of edge detectors that
>> > > generate interrupts requests or wake-up events.
>> > >
>> > > Each line can be independently configured as interrupt or wake-up source,
>> > > and triggers either on rising, falling or both edges. Each line can also
>> > > be masked independently.
>> > >
>> > > Signed-off-by: Maxime Coquelin 
>> > > Signed-off-by: Alexandre TORGUE 
>> >
>> > That all looks very reasonable now. The only remaining question is your SOB
>> > chain. Who is the author of these patches? You or Maxime? If it's Maxime,
>> > then the changelog misses a From: tag. If it's you then Maximes SOB is
>> > bogus.
>>
>> Actually Maxime wrote the main part of this driver and sent version 1 and 2 
>> of
>> the series. After Linus W. reviews, rework was required to use hierarchical
>> domain. According to Maxime, I coded the rework (adaptation to hierarchical
>> domain) and sent other version of the series.
>
> So I replace Maximes SOB with Originally-by: Does that apply to all four
> patches?
Yes, that's fine.
Alex did a lot of rework on this series, he deserves the SoB.

Thanks,
Maxime

Re: [PATCH v2 07/10] reset: stm32: add driver Kconfig option

[Maxime Coquelin]

2016-08-30 10:24 GMT+02:00 Philipp Zabel <p.za...@pengutronix.de>:
> Visible only if COMPILE_TEST is enabled, this allows to include the
> driver in build tests.
>
> Cc: Maxime Coquelin <mcoquelin.st...@gmail.com>
> Cc: Gabriel Fernandez <gabriel.fernan...@st.com>
> Reviewed-by: Masahiro Yamada <yamada.masah...@socionext.com>
> Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
> ---
>  drivers/reset/Kconfig  | 6 ++
>  drivers/reset/Makefile | 2 +-
>  2 files changed, 7 insertions(+), 1 deletion(-)

In line with what we did on other stm32 drivers:
Reviewed-by: Maxime Coquelin <mcoquelin.st...@gmail.com>

Thanks!
Maxime

Re: [PATCH v2 07/10] reset: stm32: add driver Kconfig option

[Maxime Coquelin]

2016-08-30 10:24 GMT+02:00 Philipp Zabel :
> Visible only if COMPILE_TEST is enabled, this allows to include the
> driver in build tests.
>
> Cc: Maxime Coquelin 
> Cc: Gabriel Fernandez 
> Reviewed-by: Masahiro Yamada 
> Signed-off-by: Philipp Zabel 
> ---
>  drivers/reset/Kconfig  | 6 ++
>  drivers/reset/Makefile | 2 +-
>  2 files changed, 7 insertions(+), 1 deletion(-)

In line with what we did on other stm32 drivers:
Reviewed-by: Maxime Coquelin 

Thanks!
Maxime

[PATCH] MAINTAINERS: update STM32 maintainers list

[Maxime Coquelin]

I will have less time to work on STM32 platform, so I propose
Alexandre as co-maintainer.

Alex is working in the STMicroelectronics division in charge of STM32
family, so he will have access to all technical information and
hardware.

Signed-off-by: Maxime Coquelin <mcoquelin.st...@gmail.com>
Cc: Alexandre Torgue <alexandre.tor...@st.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7304d2e37a98..4504331809ac 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1723,6 +1723,7 @@ F:drivers/ata/ahci_st.c
 
 ARM/STM32 ARCHITECTURE
 M: Maxime Coquelin <mcoquelin.st...@gmail.com>
+M: Alexandre Torgue <alexandre.tor...@st.com>
 L: linux-arm-ker...@lists.infradead.org (moderated for non-subscribers)
 S: Maintained
 T: git git://git.kernel.org/pub/scm/linux/kernel/git/mcoquelin/stm32.git
-- 
1.9.1

[PATCH] MAINTAINERS: update STM32 maintainers list

[Maxime Coquelin]

I will have less time to work on STM32 platform, so I propose
Alexandre as co-maintainer.

Alex is working in the STMicroelectronics division in charge of STM32
family, so he will have access to all technical information and
hardware.

Signed-off-by: Maxime Coquelin 
Cc: Alexandre Torgue 
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7304d2e37a98..4504331809ac 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1723,6 +1723,7 @@ F:drivers/ata/ahci_st.c
 
 ARM/STM32 ARCHITECTURE
 M: Maxime Coquelin 
+M: Alexandre Torgue 
 L: linux-arm-ker...@lists.infradead.org (moderated for non-subscribers)
 S: Maintained
 T: git git://git.kernel.org/pub/scm/linux/kernel/git/mcoquelin/stm32.git
-- 
1.9.1

[PATCH] MAINTAINERS: update STi maintainer list

[Maxime Coquelin]

Remove myself as STi maintainer as I will no longer have access to
STi platforms, and remove Srini too, who now works on other
platforms.

Patrice will manage the pull requests.

Signed-off-by: Maxime Coquelin <maxime.coque...@st.com>
Cc: Patrice Chotard <patrice.chot...@st.com>
Cc: Srinivas Kandagatla <srinivas.kandaga...@linaro.org>
Cc: Arnd Bergmann <a...@arndb.de>
---
 MAINTAINERS | 2 --
 1 file changed, 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7304d2e37a98..1ea14565d9d8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1689,8 +1689,6 @@ S:Maintained
 F: drivers/edac/altera_edac.
 
 ARM/STI ARCHITECTURE
-M: Srinivas Kandagatla <srinivas.kandaga...@gmail.com>
-M: Maxime Coquelin <maxime.coque...@st.com>
 M: Patrice Chotard <patrice.chot...@st.com>
 L: linux-arm-ker...@lists.infradead.org (moderated for non-subscribers)
 L: ker...@stlinux.com
-- 
1.9.1

[PATCH] MAINTAINERS: update STi maintainer list

[Maxime Coquelin]

Remove myself as STi maintainer as I will no longer have access to
STi platforms, and remove Srini too, who now works on other
platforms.

Patrice will manage the pull requests.

Signed-off-by: Maxime Coquelin 
Cc: Patrice Chotard 
Cc: Srinivas Kandagatla 
Cc: Arnd Bergmann 
---
 MAINTAINERS | 2 --
 1 file changed, 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7304d2e37a98..1ea14565d9d8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1689,8 +1689,6 @@ S:Maintained
 F: drivers/edac/altera_edac.
 
 ARM/STI ARCHITECTURE
-M: Srinivas Kandagatla 
-M: Maxime Coquelin 
 M: Patrice Chotard 
 L: linux-arm-ker...@lists.infradead.org (moderated for non-subscribers)
 L: ker...@stlinux.com
-- 
1.9.1

Re: [PATCH V2 12/63] clocksource/drivers/armv7m_systick: Convert init function to return error

[Maxime Coquelin]

2016-06-16 23:26 GMT+02:00 Daniel Lezcano <daniel.lezc...@linaro.org>:
> The init functions do not return any error. They behave as the following:
>
>  - panic, thus leading to a kernel crash while another timer may work and
>make the system boot up correctly
>
>  or
>
>  - print an error and let the caller unaware if the state of the system
>
> Change that by converting the init functions to return an error conforming
> to the CLOCKSOURCE_OF_RET prototype.
>
> Proper error handling (rollback, errno value) will be changed later case
> by case, thus this change just return back an error or success in the init
> function.
>
> Signed-off-by: Daniel Lezcano <daniel.lezc...@linaro.org>
> ---
>  drivers/clocksource/armv7m_systick.c | 18 --
>  1 file changed, 12 insertions(+), 6 deletions(-)

Acked-by: Maxime Coquelin <mcoquelin.st...@gmail.com>

Thanks!
Maxime

Re: [PATCH V2 12/63] clocksource/drivers/armv7m_systick: Convert init function to return error

[Maxime Coquelin]

2016-06-16 23:26 GMT+02:00 Daniel Lezcano :
> The init functions do not return any error. They behave as the following:
>
>  - panic, thus leading to a kernel crash while another timer may work and
>make the system boot up correctly
>
>  or
>
>  - print an error and let the caller unaware if the state of the system
>
> Change that by converting the init functions to return an error conforming
> to the CLOCKSOURCE_OF_RET prototype.
>
> Proper error handling (rollback, errno value) will be changed later case
> by case, thus this change just return back an error or success in the init
> function.
>
> Signed-off-by: Daniel Lezcano 
> ---
>  drivers/clocksource/armv7m_systick.c | 18 --
>  1 file changed, 12 insertions(+), 6 deletions(-)

Acked-by: Maxime Coquelin 

Thanks!
Maxime

Re: [PATCH V2 08/63] clocksource/drivers/st_lpc: Convert init function to return error

[Maxime Coquelin]

On 06/16/2016 11:26 PM, Daniel Lezcano wrote:

The init functions do not return any error. They behave as the following:

  - panic, thus leading to a kernel crash while another timer may work and
make the system boot up correctly

  or

  - print an error and let the caller unaware if the state of the system

Change that by converting the init functions to return an error conforming
to the CLOCKSOURCE_OF_RET prototype.

Proper error handling (rollback, errno value) will be changed later case
by case, thus this change just return back an error or success in the init
function.

Signed-off-by: Daniel Lezcano <daniel.lezc...@linaro.org>
---
  drivers/clocksource/clksrc_st_lpc.c | 22 +-
  1 file changed, 13 insertions(+), 9 deletions(-)


Acked-by: Maxime Coquelin <maxime.coque...@st.com>

Thanks!
Maxime

Re: [PATCH V2 08/63] clocksource/drivers/st_lpc: Convert init function to return error

[Maxime Coquelin]

On 06/16/2016 11:26 PM, Daniel Lezcano wrote:

The init functions do not return any error. They behave as the following:

  - panic, thus leading to a kernel crash while another timer may work and
make the system boot up correctly

  or

  - print an error and let the caller unaware if the state of the system

Change that by converting the init functions to return an error conforming
to the CLOCKSOURCE_OF_RET prototype.

Proper error handling (rollback, errno value) will be changed later case
by case, thus this change just return back an error or success in the init
function.

Signed-off-by: Daniel Lezcano 
---
  drivers/clocksource/clksrc_st_lpc.c | 22 +-
  1 file changed, 13 insertions(+), 9 deletions(-)


Acked-by: Maxime Coquelin 

Thanks!
Maxime

Re: [PATCH V2 18/63] clocksource/drivers/arm_global_timer: Convert init function to return error

[Maxime Coquelin]

On 06/16/2016 11:26 PM, Daniel Lezcano wrote:

The init functions do not return any error. They behave as the following:

   - panic, thus leading to a kernel crash while another timer may work and
make the system boot up correctly

   or

   - print an error and let the caller unaware if the state of the system

Change that by converting the init functions to return an error conforming
to the CLOCKSOURCE_OF_RET prototype.

Proper error handling (rollback, errno value) will be changed later case
by case, thus this change just return back an error or success in the init
function.

Signed-off-by: Daniel Lezcano <daniel.lezc...@linaro.org>
---
  drivers/clocksource/arm_global_timer.c | 28 ++--
  1 file changed, 18 insertions(+), 10 deletions(-)




Acked-by: Maxime Coquelin <maxime.coque...@st.com>

Thanks!
Maxime

Re: [PATCH V2 18/63] clocksource/drivers/arm_global_timer: Convert init function to return error

[Maxime Coquelin]

On 06/16/2016 11:26 PM, Daniel Lezcano wrote:

The init functions do not return any error. They behave as the following:

   - panic, thus leading to a kernel crash while another timer may work and
make the system boot up correctly

   or

   - print an error and let the caller unaware if the state of the system

Change that by converting the init functions to return an error conforming
to the CLOCKSOURCE_OF_RET prototype.

Proper error handling (rollback, errno value) will be changed later case
by case, thus this change just return back an error or success in the init
function.

Signed-off-by: Daniel Lezcano 
---
  drivers/clocksource/arm_global_timer.c | 28 ++--
  1 file changed, 18 insertions(+), 10 deletions(-)




Acked-by: Maxime Coquelin 

Thanks!
Maxime

Re: [PATCH V2 47/63] clocksource/drivers/timer-stm32: Convert init function to return error

[Maxime Coquelin]

2016-06-16 23:27 GMT+02:00 Daniel Lezcano <daniel.lezc...@linaro.org>:
> The init functions do not return any error. They behave as the following:
>
>   - panic, thus leading to a kernel crash while another timer may work and
>make the system boot up correctly
>
>   or
>
>   - print an error and let the caller unaware if the state of the system
>
> Change that by converting the init functions to return an error conforming
> to the CLOCKSOURCE_OF_RET prototype.
>
> Proper error handling (rollback, errno value) will be changed later case
> by case, thus this change just return back an error or success in the init
> function.
>
> Signed-off-by: Daniel Lezcano <daniel.lezc...@linaro.org>
> ---
>  drivers/clocksource/timer-stm32.c | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)


Acked-by: Maxime Coquelin <mcoquelin.st...@gmail.com>

Thanks!
Maxime

Re: [PATCH V2 47/63] clocksource/drivers/timer-stm32: Convert init function to return error

[Maxime Coquelin]

2016-06-16 23:27 GMT+02:00 Daniel Lezcano :
> The init functions do not return any error. They behave as the following:
>
>   - panic, thus leading to a kernel crash while another timer may work and
>make the system boot up correctly
>
>   or
>
>   - print an error and let the caller unaware if the state of the system
>
> Change that by converting the init functions to return an error conforming
> to the CLOCKSOURCE_OF_RET prototype.
>
> Proper error handling (rollback, errno value) will be changed later case
> by case, thus this change just return back an error or success in the init
> function.
>
> Signed-off-by: Daniel Lezcano 
> ---
>  drivers/clocksource/timer-stm32.c | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)


Acked-by: Maxime Coquelin 

Thanks!
Maxime

[PATCH v4.7-rc] ARM: dts: STi: stih407-family: Disable reserved-memory co-processor nodes

[Maxime Coquelin]

From: Lee Jones <lee.jo...@linaro.org>

This patch fixes a non-booting issue in Mainline.

When booting with a compressed kernel, we need to be careful how we
populate memory close to DDR start.  AUTO_ZRELADDR is enabled by default
in multi-arch enabled configurations, which place some restrictions on
where the kernel is placed and where it will be uncompressed to on boot.

AUTO_ZRELADDR takes the decompressor code's start address and masks out
the bottom 28 bits to obtain an address to uncompress the kernel to
(thus a load address of 0x4200 means that the kernel will be
uncompressed to 0x4000 i.e. DDR START on this platform).

Even changing the load address to after the co-processor's shared memory
won't render a booting platform, since the AUTO_ZRELADDR algorithm still
ensures the kernel is uncompressed into memory shared with the first
co-processor (0x4000).

Another option would be to move loading to 0x4A00, since this will
mean the decompressor will decompress the kernel to 0x4800. However,
this would mean a large chunk (0x4400 => 0x4800 (64MB)) of
memory would essentially be wasted for no good reason.

Until we can work with ST to find a suitable memory location to
relocate co-processor shared memory, let's disable the shared memory
nodes.  This will ensure a working platform in the mean time.

NB: The more observant of you will notice that we're leaving the DMU
shared memory node enabled; this is because a) it is the only one in
active use at the time of this writing and b) it is not affected by
the current default behaviour which is causing issues.

Fixes: fe135c6 (ARM: dts: STiH407: Move over to using the 'reserved-memory' API 
for obtaining DMA memory)
Signed-off-by: Lee Jones <lee.jo...@linaro.org>
Reviewed-by Peter Griffin <peter.grif...@linaro.org>
Signed-off-by: Maxime Coquelin <maxime.coque...@st.com>
---
 arch/arm/boot/dts/stih407-family.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/boot/dts/stih407-family.dtsi 
b/arch/arm/boot/dts/stih407-family.dtsi
index ad8ba10764a3..d294e82447a2 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -24,18 +24,21 @@
compatible = "shared-dma-pool";
reg = <0x4000 0x0100>;
no-map;
+   status = "disabled";
};
 
gp1_reserved: rproc@4100 {
compatible = "shared-dma-pool";
reg = <0x4100 0x0100>;
no-map;
+   status = "disabled";
};
 
audio_reserved: rproc@4200 {
compatible = "shared-dma-pool";
reg = <0x4200 0x0100>;
no-map;
+   status = "disabled";
};
 
dmu_reserved: rproc@4300 {
-- 
1.9.1

[PATCH v4.7-rc] ARM: dts: STi: stih407-family: Disable reserved-memory co-processor nodes

[Maxime Coquelin]

From: Lee Jones 

This patch fixes a non-booting issue in Mainline.

When booting with a compressed kernel, we need to be careful how we
populate memory close to DDR start.  AUTO_ZRELADDR is enabled by default
in multi-arch enabled configurations, which place some restrictions on
where the kernel is placed and where it will be uncompressed to on boot.

AUTO_ZRELADDR takes the decompressor code's start address and masks out
the bottom 28 bits to obtain an address to uncompress the kernel to
(thus a load address of 0x4200 means that the kernel will be
uncompressed to 0x4000 i.e. DDR START on this platform).

Even changing the load address to after the co-processor's shared memory
won't render a booting platform, since the AUTO_ZRELADDR algorithm still
ensures the kernel is uncompressed into memory shared with the first
co-processor (0x4000).

Another option would be to move loading to 0x4A00, since this will
mean the decompressor will decompress the kernel to 0x4800. However,
this would mean a large chunk (0x4400 => 0x4800 (64MB)) of
memory would essentially be wasted for no good reason.

Until we can work with ST to find a suitable memory location to
relocate co-processor shared memory, let's disable the shared memory
nodes.  This will ensure a working platform in the mean time.

NB: The more observant of you will notice that we're leaving the DMU
shared memory node enabled; this is because a) it is the only one in
active use at the time of this writing and b) it is not affected by
the current default behaviour which is causing issues.

Fixes: fe135c6 (ARM: dts: STiH407: Move over to using the 'reserved-memory' API 
for obtaining DMA memory)
Signed-off-by: Lee Jones 
Reviewed-by Peter Griffin 
Signed-off-by: Maxime Coquelin 
---
 arch/arm/boot/dts/stih407-family.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/boot/dts/stih407-family.dtsi 
b/arch/arm/boot/dts/stih407-family.dtsi
index ad8ba10764a3..d294e82447a2 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -24,18 +24,21 @@
compatible = "shared-dma-pool";
reg = <0x4000 0x0100>;
no-map;
+   status = "disabled";
};
 
gp1_reserved: rproc@4100 {
compatible = "shared-dma-pool";
reg = <0x4100 0x0100>;
no-map;
+   status = "disabled";
};
 
audio_reserved: rproc@4200 {
compatible = "shared-dma-pool";
reg = <0x4200 0x0100>;
no-map;
+   status = "disabled";
};
 
dmu_reserved: rproc@4300 {
-- 
1.9.1

Re: [PATCH] ARM: dts: STi: stih407-family: Disable reserved-memory co-processor nodes

[Maxime Coquelin]

On 06/07/2016 10:37 AM, Lee Jones wrote:

This patch fixes a non-booting issue in Mainline.

When booting with a compressed kernel, we need to be careful how we
populate memory close to DDR start.  AUTO_ZRELADDR is enabled by default
in multi-arch enabled configurations, which place some restrictions on
where the kernel is placed and where it will be uncompressed to on boot.

AUTO_ZRELADDR takes the decompressor code's start address and masks out
the bottom 28 bits to obtain an address to uncompress the kernel to
(thus a load address of 0x4200 means that the kernel will be
uncompressed to 0x4000 i.e. DDR START on this platform).

Even changing the load address to after the co-processor's shared memory
won't render a booting platform, since the AUTO_ZRELADDR algorithm still
ensures the kernel is uncompressed into memory shared with the first
co-processor (0x4000).

Another option would be to move loading to 0x4A00, since this will
mean the decompressor will decompress the kernel to 0x4800. However,
this would mean a large chunk (0x4400 => 0x4800 (64MB)) of
memory would essentially be wasted for no good reason.

Until we can work with ST to find a suitable memory location to
relocate co-processor shared memory, let's disable the shared memory
nodes.  This will ensure a working platform in the mean time.

NB: The more observant of you will notice that we're leaving the DMU
shared memory node enabled; this is because a) it is the only one in
active use at the time of this writing and b) it is not affected by
the current default behaviour which is causing issues.

Fixes: fe135c6 (ARM: dts: STiH407: Move over to using the 'reserved-memory' API 
for obtaining DMA memory)
Signed-off-by: Lee Jones <lee.jo...@linaro.org>
---
  arch/arm/boot/dts/stih407-family.dtsi | 3 +++
  1 file changed, 3 insertions(+)




That sounds reasonable:
Acked-by: Maxime Coquelin <maxime.coque...@st.com>

Arnd, Olof, can you pick this patch directly in -rc fixes,
as it prevents STi boards to boot correctly?

Thanks in advance,
Maxime

Re: [PATCH] ARM: dts: STi: stih407-family: Disable reserved-memory co-processor nodes

[Maxime Coquelin]

On 06/07/2016 10:37 AM, Lee Jones wrote:

This patch fixes a non-booting issue in Mainline.

When booting with a compressed kernel, we need to be careful how we
populate memory close to DDR start.  AUTO_ZRELADDR is enabled by default
in multi-arch enabled configurations, which place some restrictions on
where the kernel is placed and where it will be uncompressed to on boot.

AUTO_ZRELADDR takes the decompressor code's start address and masks out
the bottom 28 bits to obtain an address to uncompress the kernel to
(thus a load address of 0x4200 means that the kernel will be
uncompressed to 0x4000 i.e. DDR START on this platform).

Even changing the load address to after the co-processor's shared memory
won't render a booting platform, since the AUTO_ZRELADDR algorithm still
ensures the kernel is uncompressed into memory shared with the first
co-processor (0x4000).

Another option would be to move loading to 0x4A00, since this will
mean the decompressor will decompress the kernel to 0x4800. However,
this would mean a large chunk (0x4400 => 0x4800 (64MB)) of
memory would essentially be wasted for no good reason.

Until we can work with ST to find a suitable memory location to
relocate co-processor shared memory, let's disable the shared memory
nodes.  This will ensure a working platform in the mean time.

NB: The more observant of you will notice that we're leaving the DMU
shared memory node enabled; this is because a) it is the only one in
active use at the time of this writing and b) it is not affected by
the current default behaviour which is causing issues.

Fixes: fe135c6 (ARM: dts: STiH407: Move over to using the 'reserved-memory' API 
for obtaining DMA memory)
Signed-off-by: Lee Jones 
---
  arch/arm/boot/dts/stih407-family.dtsi | 3 +++
  1 file changed, 3 insertions(+)




That sounds reasonable:
Acked-by: Maxime Coquelin 

Arnd, Olof, can you pick this patch directly in -rc fixes,
as it prevents STi boards to boot correctly?

Thanks in advance,
Maxime

Re: [PATCH v2 3/5] ARM: dts: Add I2C1 support for STM32F429 SoC

[Maxime Coquelin]

2016-06-02 16:26 GMT+02:00 M'boumba Cedric Madianga :
> Signed-off-by: Patrice Chotard 
> Signed-off-by: M'boumba Cedric Madianga 
> ---
>  arch/arm/boot/dts/stm32f429.dtsi | 24 
>  1 file changed, 24 insertions(+)
>
> diff --git a/arch/arm/boot/dts/stm32f429.dtsi 
> b/arch/arm/boot/dts/stm32f429.dtsi
> index 434d4b9..d5857eb 100644
> --- a/arch/arm/boot/dts/stm32f429.dtsi
> +++ b/arch/arm/boot/dts/stm32f429.dtsi
> @@ -323,6 +323,18 @@
> slew-rate = <2>;
> };
> };
> +
> +   i2c1_sda_pin: i2c1_sda@0 {
> +   pins {
> +   pinmux = 
> ;
> +   drive-open-drain;
> +   };
> +   };
> +   i2c1_scl_pin: i2c1_scl@0 {
> +   pins {
> +   pinmux = 
> ;
> +   };
> +   };
> };
Shouldn't be preferrable to group the two functions in a single one,
as done for usart config in this file?:

usart1_pins_a: usart1@0 {
pins1 {
pinmux = ;
bias-disable;
drive-push-pull;
slew-rate = <0>;
};
pins2 {
pinmux = ;
bias-disable;
};
};

Also, I would prefer the phandle to be suffixed with the port number,
as multiple muxing options are available, for example "i2c1_pins_b".

Thanks,
Maxime

Re: [PATCH v2 3/5] ARM: dts: Add I2C1 support for STM32F429 SoC

[Maxime Coquelin]

2016-06-02 16:26 GMT+02:00 M'boumba Cedric Madianga :
> Signed-off-by: Patrice Chotard 
> Signed-off-by: M'boumba Cedric Madianga 
> ---
>  arch/arm/boot/dts/stm32f429.dtsi | 24 
>  1 file changed, 24 insertions(+)
>
> diff --git a/arch/arm/boot/dts/stm32f429.dtsi 
> b/arch/arm/boot/dts/stm32f429.dtsi
> index 434d4b9..d5857eb 100644
> --- a/arch/arm/boot/dts/stm32f429.dtsi
> +++ b/arch/arm/boot/dts/stm32f429.dtsi
> @@ -323,6 +323,18 @@
> slew-rate = <2>;
> };
> };
> +
> +   i2c1_sda_pin: i2c1_sda@0 {
> +   pins {
> +   pinmux = 
> ;
> +   drive-open-drain;
> +   };
> +   };
> +   i2c1_scl_pin: i2c1_scl@0 {
> +   pins {
> +   pinmux = 
> ;
> +   };
> +   };
> };
Shouldn't be preferrable to group the two functions in a single one,
as done for usart config in this file?:

usart1_pins_a: usart1@0 {
pins1 {
pinmux = ;
bias-disable;
drive-push-pull;
slew-rate = <0>;
};
pins2 {
pinmux = ;
bias-disable;
};
};

Also, I would prefer the phandle to be suffixed with the port number,
as multiple muxing options are available, for example "i2c1_pins_b".

Thanks,
Maxime

Re: [PATCH 2/5] i2c: Add STM32F4 I2C driver

[Maxime Coquelin]

Hi Cedric,

2016-06-02 17:35 GMT+02:00 M'boumba Cedric Madianga :
> Hi,
>
>>> +
>>> +/**
>>> + * stm32f4_i2c_xfer() - Transfer combined I2C message
>>> + * @i2c_adap: Adapter pointer to the controller
>>> + * @msgs: Pointer to data to be written.
>>> + * @num: Number of messages to be executed
>>> + */
>>> +static int stm32f4_i2c_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg 
>>> msgs[],
>>> +   int num)
>>> +{
>>> +   struct stm32f4_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
>>> +   int ret, i;
>>> +
>>> +   i2c_dev->busy = true;
>>> +
>>> +   ret = clk_prepare_enable(i2c_dev->clk);
>>> +   if (ret) {
>>> +   dev_err(i2c_dev->dev, "Failed to prepare_enable clock\n");
>>> +   return ret;
>>> +   }
>>> +
>>> +   stm32f4_i2c_hw_config(i2c_dev);
>> Maybe you could call this only at probe and resume time?
>> You would save some register accesses.
> Some clarification about this point.
> We need to call stm32f4_i2c_hw_config before each I2C transfer as at
> the end of I2C communication the peripheral is automatically disabled
> and configuration registers are reset.

Ok, but I wonder how the IP knows this is the last i2c message to be sent?
Or maybe it gets re-initialized as soon as the clk is disabled?

Thanks for the inputs,
Maxime

Re: [PATCH 2/5] i2c: Add STM32F4 I2C driver

[Maxime Coquelin]

Hi Cedric,

2016-06-02 17:35 GMT+02:00 M'boumba Cedric Madianga :
> Hi,
>
>>> +
>>> +/**
>>> + * stm32f4_i2c_xfer() - Transfer combined I2C message
>>> + * @i2c_adap: Adapter pointer to the controller
>>> + * @msgs: Pointer to data to be written.
>>> + * @num: Number of messages to be executed
>>> + */
>>> +static int stm32f4_i2c_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg 
>>> msgs[],
>>> +   int num)
>>> +{
>>> +   struct stm32f4_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
>>> +   int ret, i;
>>> +
>>> +   i2c_dev->busy = true;
>>> +
>>> +   ret = clk_prepare_enable(i2c_dev->clk);
>>> +   if (ret) {
>>> +   dev_err(i2c_dev->dev, "Failed to prepare_enable clock\n");
>>> +   return ret;
>>> +   }
>>> +
>>> +   stm32f4_i2c_hw_config(i2c_dev);
>> Maybe you could call this only at probe and resume time?
>> You would save some register accesses.
> Some clarification about this point.
> We need to call stm32f4_i2c_hw_config before each I2C transfer as at
> the end of I2C communication the peripheral is automatically disabled
> and configuration registers are reset.

Ok, but I wonder how the IP knows this is the last i2c message to be sent?
Or maybe it gets re-initialized as soon as the clk is disabled?

Thanks for the inputs,
Maxime

Re: [PATCH 2/5] i2c: Add STM32F4 I2C driver

[Maxime Coquelin]

2016-06-01 16:01 GMT+02:00 M'boumba Cedric Madianga :
> Hi Maxime,
>
 +static void stm32f4_i2c_set_speed_mode(struct stm32f4_i2c_dev *i2c_dev)
 +{
 +   struct stm32f4_i2c_timings *t = _timings[i2c_dev->speed];
 +   u32 ccr, val, clk_rate;
 +
 +   ccr = readl_relaxed(i2c_dev->base + STM32F4_I2C_CCR);
 +   ccr &= ~(STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY |
 +STM32F4_I2C_CCR_CCR_MASK);
 +
 +   clk_rate = clk_get_rate(i2c_dev->clk);
 +
 +   switch (i2c_dev->speed) {
 +   case STM32F4_I2C_SPEED_STANDARD:
 +   val = clk_rate / t->rate * 2;
 +   if (val < STM32F4_I2C_MIN_CCR)
 +   ccr |= STM32F4_I2C_CCR_CCR(STM32F4_I2C_MIN_CCR);
 +   else
 +   ccr |= STM32F4_I2C_CCR_CCR(val);
 +   break;
 +   case STM32F4_I2C_SPEED_FAST:
 +   ccr |= STM32F4_I2C_CCR_FS;
 +   if (t->duty) {
 +   ccr |= STM32F4_I2C_CCR_DUTY;
 +   ccr |= STM32F4_I2C_CCR_CCR(clk_rate / t->rate * 
 25);
 +   } else {
 +   ccr |= STM32F4_I2C_CCR_CCR(clk_rate / t->rate * 3);
 +   }
>>> Is it really useful since duty seems to always be 0?
>> Agree, I will rework it by directly set duty at 0 in the register.
>
> Contrary to what I wrote previously, the duty has to be set for FAST
> Mode to reach 400khz.
> So, I am going to keep the timing struct and set duty to 1 for FAST mode

Ok, That's fine by me.

Re: [PATCH 2/5] i2c: Add STM32F4 I2C driver

[Maxime Coquelin]

2016-06-01 16:01 GMT+02:00 M'boumba Cedric Madianga :
> Hi Maxime,
>
 +static void stm32f4_i2c_set_speed_mode(struct stm32f4_i2c_dev *i2c_dev)
 +{
 +   struct stm32f4_i2c_timings *t = _timings[i2c_dev->speed];
 +   u32 ccr, val, clk_rate;
 +
 +   ccr = readl_relaxed(i2c_dev->base + STM32F4_I2C_CCR);
 +   ccr &= ~(STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY |
 +STM32F4_I2C_CCR_CCR_MASK);
 +
 +   clk_rate = clk_get_rate(i2c_dev->clk);
 +
 +   switch (i2c_dev->speed) {
 +   case STM32F4_I2C_SPEED_STANDARD:
 +   val = clk_rate / t->rate * 2;
 +   if (val < STM32F4_I2C_MIN_CCR)
 +   ccr |= STM32F4_I2C_CCR_CCR(STM32F4_I2C_MIN_CCR);
 +   else
 +   ccr |= STM32F4_I2C_CCR_CCR(val);
 +   break;
 +   case STM32F4_I2C_SPEED_FAST:
 +   ccr |= STM32F4_I2C_CCR_FS;
 +   if (t->duty) {
 +   ccr |= STM32F4_I2C_CCR_DUTY;
 +   ccr |= STM32F4_I2C_CCR_CCR(clk_rate / t->rate * 
 25);
 +   } else {
 +   ccr |= STM32F4_I2C_CCR_CCR(clk_rate / t->rate * 3);
 +   }
>>> Is it really useful since duty seems to always be 0?
>> Agree, I will rework it by directly set duty at 0 in the register.
>
> Contrary to what I wrote previously, the duty has to be set for FAST
> Mode to reach 400khz.
> So, I am going to keep the timing struct and set duty to 1 for FAST mode

Ok, That's fine by me.

[PATCH] hwrng: stm32: fix maybe uninitialized variable warning

[Maxime Coquelin]

This patch fixes the following warning:
drivers/char/hw_random/stm32-rng.c: In function 'stm32_rng_read':
drivers/char/hw_random/stm32-rng.c:82:19: warning: 'sr' may be used
uninitialized in this function

Reported-by: Sudip Mukherjee <sudip.mukher...@codethink.co.uk>
Suggested-by: Arnd Bergmann <a...@arndb.de>
Cc: Daniel Thompson <daniel.thomp...@linaro.org>
Signed-off-by: Maxime Coquelin <mcoquelin.st...@gmail.com>
---
 drivers/char/hw_random/stm32-rng.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/char/hw_random/stm32-rng.c 
b/drivers/char/hw_random/stm32-rng.c
index 92a810648bd0..63d84e6f1891 100644
--- a/drivers/char/hw_random/stm32-rng.c
+++ b/drivers/char/hw_random/stm32-rng.c
@@ -69,8 +69,12 @@ static int stm32_rng_read(struct hwrng *rng, void *data, 
size_t max, bool wait)
}
 
/* If error detected or data not ready... */
-   if (sr != RNG_SR_DRDY)
+   if (sr != RNG_SR_DRDY) {
+   if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
+   "bad RNG status - %x\n", sr))
+   writel_relaxed(0, priv->base + RNG_SR);
break;
+   }
 
*(u32 *)data = readl_relaxed(priv->base + RNG_DR);
 
@@ -79,10 +83,6 @@ static int stm32_rng_read(struct hwrng *rng, void *data, 
size_t max, bool wait)
max -= sizeof(u32);
}
 
-   if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
- "bad RNG status - %x\n", sr))
-   writel_relaxed(0, priv->base + RNG_SR);
-
pm_runtime_mark_last_busy((struct device *) priv->rng.priv);
pm_runtime_put_sync_autosuspend((struct device *) priv->rng.priv);
 
-- 
1.9.1

[PATCH] hwrng: stm32: fix maybe uninitialized variable warning

[Maxime Coquelin]

This patch fixes the following warning:
drivers/char/hw_random/stm32-rng.c: In function 'stm32_rng_read':
drivers/char/hw_random/stm32-rng.c:82:19: warning: 'sr' may be used
uninitialized in this function

Reported-by: Sudip Mukherjee 
Suggested-by: Arnd Bergmann 
Cc: Daniel Thompson 
Signed-off-by: Maxime Coquelin 
---
 drivers/char/hw_random/stm32-rng.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/char/hw_random/stm32-rng.c 
b/drivers/char/hw_random/stm32-rng.c
index 92a810648bd0..63d84e6f1891 100644
--- a/drivers/char/hw_random/stm32-rng.c
+++ b/drivers/char/hw_random/stm32-rng.c
@@ -69,8 +69,12 @@ static int stm32_rng_read(struct hwrng *rng, void *data, 
size_t max, bool wait)
}
 
/* If error detected or data not ready... */
-   if (sr != RNG_SR_DRDY)
+   if (sr != RNG_SR_DRDY) {
+   if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
+   "bad RNG status - %x\n", sr))
+   writel_relaxed(0, priv->base + RNG_SR);
break;
+   }
 
*(u32 *)data = readl_relaxed(priv->base + RNG_DR);
 
@@ -79,10 +83,6 @@ static int stm32_rng_read(struct hwrng *rng, void *data, 
size_t max, bool wait)
max -= sizeof(u32);
}
 
-   if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
- "bad RNG status - %x\n", sr))
-   writel_relaxed(0, priv->base + RNG_SR);
-
pm_runtime_mark_last_busy((struct device *) priv->rng.priv);
pm_runtime_put_sync_autosuspend((struct device *) priv->rng.priv);
 
-- 
1.9.1

Re: [PATCH v2] pinctrl: stm32: factorize stm32_pconf_input/output_get()

[Maxime Coquelin]

On 05/24/2016 01:57 PM, patrice.chot...@st.com wrote:

From: Patrice Chotard<patrice.chot...@st.com>

As these 2 functions code are 95% similar, factorize them.

Signed-off-by: Patrice Chotard<patrice.chot...@st.com>
---
  drivers/pinctrl/stm32/pinctrl-stm32.c | 31 ++-
  1 file changed, 10 insertions(+), 21 deletions(-)


Acked-by: Maxime Coquelin <maxime.coque...@st.com>

Thanks!
Maxime

Re: [PATCH v2] pinctrl: stm32: factorize stm32_pconf_input/output_get()

[Maxime Coquelin]

On 05/24/2016 01:57 PM, patrice.chot...@st.com wrote:

From: Patrice Chotard

As these 2 functions code are 95% similar, factorize them.

Signed-off-by: Patrice Chotard
---
  drivers/pinctrl/stm32/pinctrl-stm32.c | 31 ++-
  1 file changed, 10 insertions(+), 21 deletions(-)


Acked-by: Maxime Coquelin 

Thanks!
Maxime

Re: [PATCH] hwrng: stm32 - fix build warning

[Maxime Coquelin]

2016-05-24 12:09 GMT+02:00 Daniel Thompson <daniel.thomp...@linaro.org>:
> On 24/05/16 09:50, Maxime Coquelin wrote:
>>
>> diff --git a/drivers/char/hw_random/stm32-rng.c
>> b/drivers/char/hw_random/stm32-rng.c
>> index 92a810648bd0..2a0fc90e4dc3 100644
>> --- a/drivers/char/hw_random/stm32-rng.c
>> +++ b/drivers/char/hw_random/stm32-rng.c
>> @@ -68,6 +68,10 @@ static int stm32_rng_read(struct hwrng *rng, void
>> *data, size_t max, bool wait)
>> } while (!sr && --timeout);
>> }
>>
>> +   if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
>> +   "bad RNG status - %x\n", sr))
>> +   writel_relaxed(0, priv->base + RNG_SR);
>> +
>> /* If error detected or data not ready... */
>> if (sr != RNG_SR_DRDY)
>> break;
>
>
> Minor quibble but I might prefer that the error handling/recovery actually
> be put on the error path itself (included in the if (sr != RNG_SR_DRDY) ).
Yes, it would be better.

Regards,
Maxime

Re: [PATCH] hwrng: stm32 - fix build warning

[Maxime Coquelin]

2016-05-24 12:09 GMT+02:00 Daniel Thompson :
> On 24/05/16 09:50, Maxime Coquelin wrote:
>>
>> diff --git a/drivers/char/hw_random/stm32-rng.c
>> b/drivers/char/hw_random/stm32-rng.c
>> index 92a810648bd0..2a0fc90e4dc3 100644
>> --- a/drivers/char/hw_random/stm32-rng.c
>> +++ b/drivers/char/hw_random/stm32-rng.c
>> @@ -68,6 +68,10 @@ static int stm32_rng_read(struct hwrng *rng, void
>> *data, size_t max, bool wait)
>> } while (!sr && --timeout);
>> }
>>
>> +   if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
>> +   "bad RNG status - %x\n", sr))
>> +   writel_relaxed(0, priv->base + RNG_SR);
>> +
>> /* If error detected or data not ready... */
>> if (sr != RNG_SR_DRDY)
>> break;
>
>
> Minor quibble but I might prefer that the error handling/recovery actually
> be put on the error path itself (included in the if (sr != RNG_SR_DRDY) ).
Yes, it would be better.

Regards,
Maxime

Re: [PATCH] hwrng: stm32 - fix build warning

[Maxime Coquelin]

2016-05-24 10:58 GMT+02:00 Arnd Bergmann <a...@arndb.de>:
> On Tuesday, May 24, 2016 10:50:17 AM CEST Maxime Coquelin wrote:
>> diff --git a/drivers/char/hw_random/stm32-rng.c
>> b/drivers/char/hw_random/stm32-rng.c
>> index 92a810648bd0..2a0fc90e4dc3 100644
>> --- a/drivers/char/hw_random/stm32-rng.c
>> +++ b/drivers/char/hw_random/stm32-rng.c
>> @@ -68,6 +68,10 @@ static int stm32_rng_read(struct hwrng *rng, void
>> *data, size_t max, bool wait)
>> } while (!sr && --timeout);
>> }
>>
>> +   if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
>> +   "bad RNG status - %x\n", sr))
>> +   writel_relaxed(0, priv->base + RNG_SR);
>> +
>> /* If error detected or data not ready... */
>> if (sr != RNG_SR_DRDY)
>> break;
>> @@ -79,10 +83,6 @@ static int stm32_rng_read(struct hwrng *rng, void
>> *data, size_t max, bool wait)
>> max -= sizeof(u32);
>> }
>>
>> -   if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
>> - "bad RNG status - %x\n", sr))
>> -   writel_relaxed(0, priv->base + RNG_SR);
>> -
>> pm_runtime_mark_last_busy((struct device *) priv->rng.priv);
>> pm_runtime_put_sync_autosuspend((struct device *) priv->rng.priv);
>>
>> Thanks,
>>
>
> Yes, that looks good to me.

Thanks!
Sudip, do you want to send the patch, or I manage to do it?

Maxime

Re: [PATCH] hwrng: stm32 - fix build warning

[Maxime Coquelin]

2016-05-24 10:58 GMT+02:00 Arnd Bergmann :
> On Tuesday, May 24, 2016 10:50:17 AM CEST Maxime Coquelin wrote:
>> diff --git a/drivers/char/hw_random/stm32-rng.c
>> b/drivers/char/hw_random/stm32-rng.c
>> index 92a810648bd0..2a0fc90e4dc3 100644
>> --- a/drivers/char/hw_random/stm32-rng.c
>> +++ b/drivers/char/hw_random/stm32-rng.c
>> @@ -68,6 +68,10 @@ static int stm32_rng_read(struct hwrng *rng, void
>> *data, size_t max, bool wait)
>> } while (!sr && --timeout);
>> }
>>
>> +   if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
>> +   "bad RNG status - %x\n", sr))
>> +   writel_relaxed(0, priv->base + RNG_SR);
>> +
>> /* If error detected or data not ready... */
>> if (sr != RNG_SR_DRDY)
>> break;
>> @@ -79,10 +83,6 @@ static int stm32_rng_read(struct hwrng *rng, void
>> *data, size_t max, bool wait)
>> max -= sizeof(u32);
>> }
>>
>> -   if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
>> - "bad RNG status - %x\n", sr))
>> -   writel_relaxed(0, priv->base + RNG_SR);
>> -
>> pm_runtime_mark_last_busy((struct device *) priv->rng.priv);
>> pm_runtime_put_sync_autosuspend((struct device *) priv->rng.priv);
>>
>> Thanks,
>>
>
> Yes, that looks good to me.

Thanks!
Sudip, do you want to send the patch, or I manage to do it?

Maxime

Re: [PATCH] hwrng: stm32 - fix build warning

[Maxime Coquelin]

2016-05-24 10:32 GMT+02:00 Arnd Bergmann <a...@arndb.de>:
> On Tuesday, May 24, 2016 9:59:41 AM CEST Maxime Coquelin wrote:
>> 2016-05-23 22:35 GMT+02:00 Arnd Bergmann <a...@arndb.de>:
>> > On Monday, May 23, 2016 6:14:08 PM CEST Sudip Mukherjee wrote:
>> >> diff --git a/drivers/char/hw_random/stm32-rng.c 
>> >> b/drivers/char/hw_random/stm32-rng.c
>> >> index 92a8106..0533370 100644
>> >> --- a/drivers/char/hw_random/stm32-rng.c
>> >> +++ b/drivers/char/hw_random/stm32-rng.c
>> >> @@ -52,7 +52,7 @@ static int stm32_rng_read(struct hwrng *rng, void 
>> >> *data, size_t max, bool wait)
>> >>  {
>> >>   struct stm32_rng_private *priv =
>> >>   container_of(rng, struct stm32_rng_private, rng);
>> >> - u32 sr;
>> >> + u32 sr = 0;
>> >>   int retval = 0;
>> >>
>> >>   pm_runtime_get_sync((struct device *) priv->rng.priv);
>> >
>> > Does this work as well?
>> >
>> > diff --git a/drivers/char/hw_random/stm32-rng.c 
>> > b/drivers/char/hw_random/stm32-rng.c
>> > index 92a810648bd0..5c836b0afa40 100644
>> > --- a/drivers/char/hw_random/stm32-rng.c
>> > +++ b/drivers/char/hw_random/stm32-rng.c
>> > @@ -79,7 +79,7 @@ static int stm32_rng_read(struct hwrng *rng, void *data, 
>> > size_t max, bool wait)
>> > max -= sizeof(u32);
>> > }
>> >
>> > -   if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
>> > +   if (WARN_ONCE(retval > 0 && (sr & (RNG_SR_SEIS | RNG_SR_CEIS)),
>> >   "bad RNG status - %x\n", sr))
>> > writel_relaxed(0, priv->base + RNG_SR);
>> >
>> > I think it would be nicer to not add a bogus initialization.
>> Hmm, no sure this nicer.
>> The while loop can break before retval is incremented when sr value is
>> not expected (sr != RNG_SR_DRDY).
>> In that case, we certainly want to print sr value.
>
> Ah, you are right.
>
>> Maybe the better way is just to initialize sr with status register content?
>
>>pm_runtime_get_sync((struct device *) priv->rng.priv);
>>
>>+   sr = readl_relaxed(priv->base + RNG_SR);
>>while (max > sizeof(u32)) {
>>-   sr = readl_relaxed(priv->base + RNG_SR);
>>if (!sr && wait) {
>>unsigned int timeout = RNG_TIMEOUT;
>
>
> I think that introduces a bug: you really want to read the status
> register on each loop iteration.
Actually, I read the status again at the end of the loop.
But my implementation isn't good anyway, because I read the status
register one time more every time.

>
> How about moving the error handling into the loop itself?
That would be better, indeed, but there is one problem with your below proposal:
>
> Arnd
>
>
> diff --git a/drivers/char/hw_random/stm32-rng.c 
> b/drivers/char/hw_random/stm32-rng.c
> index 92a810648bd0..fceacd809462 100644
> --- a/drivers/char/hw_random/stm32-rng.c
> +++ b/drivers/char/hw_random/stm32-rng.c
> @@ -59,6 +59,10 @@ static int stm32_rng_read(struct hwrng *rng, void *data, 
> size_t max, bool wait)
>
> while (max > sizeof(u32)) {
> sr = readl_relaxed(priv->base + RNG_SR);
> +   if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
> + "bad RNG status - %x\n", sr))
> +   writel_relaxed(0, priv->base + RNG_SR);
> +
The error handling should be moved after the last status register read.

> if (!sr && wait) {
> unsigned int timeout = RNG_TIMEOUT;
>
> @@ -79,10 +83,6 @@ static int stm32_rng_read(struct hwrng *rng, void *data, 
> size_t max, bool wait)
> max -= sizeof(u32);
> }
>
> -   if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
> - "bad RNG status - %x\n", sr))
> -   writel_relaxed(0, priv->base + RNG_SR);
> -
> pm_runtime_mark_last_busy((struct device *) priv->rng.priv);
> pm_runtime_put_sync_autosuspend((struct device *) priv->rng.priv);


diff --git a/drivers/char/hw_random/stm32-rng.c
b/drivers/char/hw_random/stm32-rng.c
index 92a810648bd0..2a0fc90e4dc3 100644
--- a/drivers/char/hw_random/stm32-rng.c
+++ b/drivers/char/hw_random/stm32-rng.c
@@ -68,6 +68,10 @@ static int stm32_rng_read(struct hwrng *rng, void
*data, size_t max, bool wait)
} while (!sr && --timeout);

Re: [PATCH] hwrng: stm32 - fix build warning

[Maxime Coquelin]

2016-05-24 10:32 GMT+02:00 Arnd Bergmann :
> On Tuesday, May 24, 2016 9:59:41 AM CEST Maxime Coquelin wrote:
>> 2016-05-23 22:35 GMT+02:00 Arnd Bergmann :
>> > On Monday, May 23, 2016 6:14:08 PM CEST Sudip Mukherjee wrote:
>> >> diff --git a/drivers/char/hw_random/stm32-rng.c 
>> >> b/drivers/char/hw_random/stm32-rng.c
>> >> index 92a8106..0533370 100644
>> >> --- a/drivers/char/hw_random/stm32-rng.c
>> >> +++ b/drivers/char/hw_random/stm32-rng.c
>> >> @@ -52,7 +52,7 @@ static int stm32_rng_read(struct hwrng *rng, void 
>> >> *data, size_t max, bool wait)
>> >>  {
>> >>   struct stm32_rng_private *priv =
>> >>   container_of(rng, struct stm32_rng_private, rng);
>> >> - u32 sr;
>> >> + u32 sr = 0;
>> >>   int retval = 0;
>> >>
>> >>   pm_runtime_get_sync((struct device *) priv->rng.priv);
>> >
>> > Does this work as well?
>> >
>> > diff --git a/drivers/char/hw_random/stm32-rng.c 
>> > b/drivers/char/hw_random/stm32-rng.c
>> > index 92a810648bd0..5c836b0afa40 100644
>> > --- a/drivers/char/hw_random/stm32-rng.c
>> > +++ b/drivers/char/hw_random/stm32-rng.c
>> > @@ -79,7 +79,7 @@ static int stm32_rng_read(struct hwrng *rng, void *data, 
>> > size_t max, bool wait)
>> > max -= sizeof(u32);
>> > }
>> >
>> > -   if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
>> > +   if (WARN_ONCE(retval > 0 && (sr & (RNG_SR_SEIS | RNG_SR_CEIS)),
>> >   "bad RNG status - %x\n", sr))
>> > writel_relaxed(0, priv->base + RNG_SR);
>> >
>> > I think it would be nicer to not add a bogus initialization.
>> Hmm, no sure this nicer.
>> The while loop can break before retval is incremented when sr value is
>> not expected (sr != RNG_SR_DRDY).
>> In that case, we certainly want to print sr value.
>
> Ah, you are right.
>
>> Maybe the better way is just to initialize sr with status register content?
>
>>pm_runtime_get_sync((struct device *) priv->rng.priv);
>>
>>+   sr = readl_relaxed(priv->base + RNG_SR);
>>while (max > sizeof(u32)) {
>>-   sr = readl_relaxed(priv->base + RNG_SR);
>>if (!sr && wait) {
>>unsigned int timeout = RNG_TIMEOUT;
>
>
> I think that introduces a bug: you really want to read the status
> register on each loop iteration.
Actually, I read the status again at the end of the loop.
But my implementation isn't good anyway, because I read the status
register one time more every time.

>
> How about moving the error handling into the loop itself?
That would be better, indeed, but there is one problem with your below proposal:
>
> Arnd
>
>
> diff --git a/drivers/char/hw_random/stm32-rng.c 
> b/drivers/char/hw_random/stm32-rng.c
> index 92a810648bd0..fceacd809462 100644
> --- a/drivers/char/hw_random/stm32-rng.c
> +++ b/drivers/char/hw_random/stm32-rng.c
> @@ -59,6 +59,10 @@ static int stm32_rng_read(struct hwrng *rng, void *data, 
> size_t max, bool wait)
>
> while (max > sizeof(u32)) {
> sr = readl_relaxed(priv->base + RNG_SR);
> +   if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
> + "bad RNG status - %x\n", sr))
> +   writel_relaxed(0, priv->base + RNG_SR);
> +
The error handling should be moved after the last status register read.

> if (!sr && wait) {
> unsigned int timeout = RNG_TIMEOUT;
>
> @@ -79,10 +83,6 @@ static int stm32_rng_read(struct hwrng *rng, void *data, 
> size_t max, bool wait)
> max -= sizeof(u32);
> }
>
> -   if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
> - "bad RNG status - %x\n", sr))
> -   writel_relaxed(0, priv->base + RNG_SR);
> -
> pm_runtime_mark_last_busy((struct device *) priv->rng.priv);
> pm_runtime_put_sync_autosuspend((struct device *) priv->rng.priv);


diff --git a/drivers/char/hw_random/stm32-rng.c
b/drivers/char/hw_random/stm32-rng.c
index 92a810648bd0..2a0fc90e4dc3 100644
--- a/drivers/char/hw_random/stm32-rng.c
+++ b/drivers/char/hw_random/stm32-rng.c
@@ -68,6 +68,10 @@ static int stm32_rng_read(struct hwrng *rng, void
*data, size_t max, bool wait)
} while (!sr && --timeout);
}

+   i

Re: [PATCH] hwrng: stm32 - fix build warning

[Maxime Coquelin]

2016-05-23 22:35 GMT+02:00 Arnd Bergmann :
> On Monday, May 23, 2016 6:14:08 PM CEST Sudip Mukherjee wrote:
>> We have been getting build warning about:
>> drivers/char/hw_random/stm32-rng.c: In function 'stm32_rng_read':
>> drivers/char/hw_random/stm32-rng.c:82:19: warning: 'sr' may be used
>>   uninitialized in this function
>>
>> On checking the code it turns out that sr can never be used
>> uninitialized as sr is getting initialized in the while loop and while
>> loop will always execute as the minimum value of max can be 32.
>> So just initialize sr to 0 while declaring it to silence the compiler.
>>
>> Signed-off-by: Sudip Mukherjee 
>> ---
>
> I notice that you are using a really old compiler. While this warning
> seems to be valid in the sense that the compiler should figure out that
> the variable might be used uninitialized, please update your toolchain
> before reporting other such problems, as gcc-4.6 had a lot more false
> positives that newer ones (5.x or 6.x) have.
>
>>
>> build log at:
>> https://travis-ci.org/sudipm-mukherjee/parport/jobs/132180906
>>
>>  drivers/char/hw_random/stm32-rng.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/hw_random/stm32-rng.c 
>> b/drivers/char/hw_random/stm32-rng.c
>> index 92a8106..0533370 100644
>> --- a/drivers/char/hw_random/stm32-rng.c
>> +++ b/drivers/char/hw_random/stm32-rng.c
>> @@ -52,7 +52,7 @@ static int stm32_rng_read(struct hwrng *rng, void *data, 
>> size_t max, bool wait)
>>  {
>>   struct stm32_rng_private *priv =
>>   container_of(rng, struct stm32_rng_private, rng);
>> - u32 sr;
>> + u32 sr = 0;
>>   int retval = 0;
>>
>>   pm_runtime_get_sync((struct device *) priv->rng.priv);
>
> Does this work as well?
>
> diff --git a/drivers/char/hw_random/stm32-rng.c 
> b/drivers/char/hw_random/stm32-rng.c
> index 92a810648bd0..5c836b0afa40 100644
> --- a/drivers/char/hw_random/stm32-rng.c
> +++ b/drivers/char/hw_random/stm32-rng.c
> @@ -79,7 +79,7 @@ static int stm32_rng_read(struct hwrng *rng, void *data, 
> size_t max, bool wait)
> max -= sizeof(u32);
> }
>
> -   if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
> +   if (WARN_ONCE(retval > 0 && (sr & (RNG_SR_SEIS | RNG_SR_CEIS)),
>   "bad RNG status - %x\n", sr))
> writel_relaxed(0, priv->base + RNG_SR);
>
> I think it would be nicer to not add a bogus initialization.
Hmm, no sure this nicer.
The while loop can break before retval is incremented when sr value is
not expected (sr != RNG_SR_DRDY).
In that case, we certainly want to print sr value.

Maybe the better way is just to initialize sr with status register content?

diff --git a/drivers/char/hw_random/stm32-rng.c
b/drivers/char/hw_random/stm32-rng.c
index 92a810648bd0..07a6659d0fe6 100644
--- a/drivers/char/hw_random/stm32-rng.c
+++ b/drivers/char/hw_random/stm32-rng.c
@@ -57,8 +57,8 @@ static int stm32_rng_read(struct hwrng *rng, void
*data, size_t max, bool wait)

pm_runtime_get_sync((struct device *) priv->rng.priv);

+   sr = readl_relaxed(priv->base + RNG_SR);
while (max > sizeof(u32)) {
-   sr = readl_relaxed(priv->base + RNG_SR);
if (!sr && wait) {
unsigned int timeout = RNG_TIMEOUT;

@@ -77,6 +77,8 @@ static int stm32_rng_read(struct hwrng *rng, void
*data, size_t max, bool wait)
retval += sizeof(u32);
data += sizeof(u32);
max -= sizeof(u32);
+
+   sr = readl_relaxed(priv->base + RNG_SR);
}

if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),


Regards,
Maxime

Re: [PATCH] hwrng: stm32 - fix build warning

[Maxime Coquelin]

2016-05-23 22:35 GMT+02:00 Arnd Bergmann :
> On Monday, May 23, 2016 6:14:08 PM CEST Sudip Mukherjee wrote:
>> We have been getting build warning about:
>> drivers/char/hw_random/stm32-rng.c: In function 'stm32_rng_read':
>> drivers/char/hw_random/stm32-rng.c:82:19: warning: 'sr' may be used
>>   uninitialized in this function
>>
>> On checking the code it turns out that sr can never be used
>> uninitialized as sr is getting initialized in the while loop and while
>> loop will always execute as the minimum value of max can be 32.
>> So just initialize sr to 0 while declaring it to silence the compiler.
>>
>> Signed-off-by: Sudip Mukherjee 
>> ---
>
> I notice that you are using a really old compiler. While this warning
> seems to be valid in the sense that the compiler should figure out that
> the variable might be used uninitialized, please update your toolchain
> before reporting other such problems, as gcc-4.6 had a lot more false
> positives that newer ones (5.x or 6.x) have.
>
>>
>> build log at:
>> https://travis-ci.org/sudipm-mukherjee/parport/jobs/132180906
>>
>>  drivers/char/hw_random/stm32-rng.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/hw_random/stm32-rng.c 
>> b/drivers/char/hw_random/stm32-rng.c
>> index 92a8106..0533370 100644
>> --- a/drivers/char/hw_random/stm32-rng.c
>> +++ b/drivers/char/hw_random/stm32-rng.c
>> @@ -52,7 +52,7 @@ static int stm32_rng_read(struct hwrng *rng, void *data, 
>> size_t max, bool wait)
>>  {
>>   struct stm32_rng_private *priv =
>>   container_of(rng, struct stm32_rng_private, rng);
>> - u32 sr;
>> + u32 sr = 0;
>>   int retval = 0;
>>
>>   pm_runtime_get_sync((struct device *) priv->rng.priv);
>
> Does this work as well?
>
> diff --git a/drivers/char/hw_random/stm32-rng.c 
> b/drivers/char/hw_random/stm32-rng.c
> index 92a810648bd0..5c836b0afa40 100644
> --- a/drivers/char/hw_random/stm32-rng.c
> +++ b/drivers/char/hw_random/stm32-rng.c
> @@ -79,7 +79,7 @@ static int stm32_rng_read(struct hwrng *rng, void *data, 
> size_t max, bool wait)
> max -= sizeof(u32);
> }
>
> -   if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
> +   if (WARN_ONCE(retval > 0 && (sr & (RNG_SR_SEIS | RNG_SR_CEIS)),
>   "bad RNG status - %x\n", sr))
> writel_relaxed(0, priv->base + RNG_SR);
>
> I think it would be nicer to not add a bogus initialization.
Hmm, no sure this nicer.
The while loop can break before retval is incremented when sr value is
not expected (sr != RNG_SR_DRDY).
In that case, we certainly want to print sr value.

Maybe the better way is just to initialize sr with status register content?

diff --git a/drivers/char/hw_random/stm32-rng.c
b/drivers/char/hw_random/stm32-rng.c
index 92a810648bd0..07a6659d0fe6 100644
--- a/drivers/char/hw_random/stm32-rng.c
+++ b/drivers/char/hw_random/stm32-rng.c
@@ -57,8 +57,8 @@ static int stm32_rng_read(struct hwrng *rng, void
*data, size_t max, bool wait)

pm_runtime_get_sync((struct device *) priv->rng.priv);

+   sr = readl_relaxed(priv->base + RNG_SR);
while (max > sizeof(u32)) {
-   sr = readl_relaxed(priv->base + RNG_SR);
if (!sr && wait) {
unsigned int timeout = RNG_TIMEOUT;

@@ -77,6 +77,8 @@ static int stm32_rng_read(struct hwrng *rng, void
*data, size_t max, bool wait)
retval += sizeof(u32);
data += sizeof(u32);
max -= sizeof(u32);
+
+   sr = readl_relaxed(priv->base + RNG_SR);
}

if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),


Regards,
Maxime

Re: [RESEND PATCH v6 0/6] Add Ethernet support on STM32F429

[Maxime Coquelin]

2016-05-18 11:31 GMT+02:00 Arnd Bergmann <a...@arndb.de>:
> On Wednesday 18 May 2016 09:48:53 Maxime Coquelin wrote:
>> 2016-05-17 18:25 GMT+02:00 David Miller <da...@davemloft.net>:
>> > From: Maxime Coquelin <mcoquelin.st...@gmail.com>
>> > Date: Tue, 17 May 2016 11:20:16 +0200
>> >
>> >> Hi David,
>> >>
>> >> 2016-05-09 21:06 GMT+02:00 David Miller <da...@davemloft.net>:
>> >>> From: Alexandre TORGUE <alexandre.tor...@gmail.com>
>> >>> Date: Mon,  9 May 2016 12:31:33 +0200
>> >>>
>> >>>> STM32F429 Chip embeds a Synopsys 3.50a MAC IP.
>> >>>> This series:
>> >>>>  -enhance current stmmac driver to control it (code already
>> >>>> available) and adds basic glue for STM32F429 chip.
>> >>>>  -Enable basic Net config in kernel.
>> >>>
>> >>> I assume this will go via the ARM tree, for the networking bits:
>> >> I would expect patches 1, 2 & 3 to got via your tree, to avoid conflicts.
>> >
>> > I don't think putting them all via the ARM tree is going to create much
>> > in the way of conflicts, and right now during the merge window offloading
>> > that work from me would really help my backlog a lot.
>>
>> Ok, I understand this is not the best time to pick it.
>>
>> Arnd, Olof & Kevin, would you accept to pick the series in your tree?
>>
>
> It's too late for v4.7 for us too, please pick up the arch/arm patches
> in your normal stm32 tree and send them to us along with any other changes
> you may have, and resend the driver by itself to netdev time after the
> merge window.
>
> The binding document can go either way, with the dts changes or with
> the driver. I see no dependencies between the patches, so we just need
> to land them all in v4.8.

Ok, this is good for me.
No problem to wait for v4.8.
I will take the arch/arm patches, and David the drivers/net/ ones once
v4.7-rc1 is out.

Thanks for your time,
Maxime

Re: [RESEND PATCH v6 0/6] Add Ethernet support on STM32F429

[Maxime Coquelin]

2016-05-18 11:31 GMT+02:00 Arnd Bergmann :
> On Wednesday 18 May 2016 09:48:53 Maxime Coquelin wrote:
>> 2016-05-17 18:25 GMT+02:00 David Miller :
>> > From: Maxime Coquelin 
>> > Date: Tue, 17 May 2016 11:20:16 +0200
>> >
>> >> Hi David,
>> >>
>> >> 2016-05-09 21:06 GMT+02:00 David Miller :
>> >>> From: Alexandre TORGUE 
>> >>> Date: Mon,  9 May 2016 12:31:33 +0200
>> >>>
>> >>>> STM32F429 Chip embeds a Synopsys 3.50a MAC IP.
>> >>>> This series:
>> >>>>  -enhance current stmmac driver to control it (code already
>> >>>> available) and adds basic glue for STM32F429 chip.
>> >>>>  -Enable basic Net config in kernel.
>> >>>
>> >>> I assume this will go via the ARM tree, for the networking bits:
>> >> I would expect patches 1, 2 & 3 to got via your tree, to avoid conflicts.
>> >
>> > I don't think putting them all via the ARM tree is going to create much
>> > in the way of conflicts, and right now during the merge window offloading
>> > that work from me would really help my backlog a lot.
>>
>> Ok, I understand this is not the best time to pick it.
>>
>> Arnd, Olof & Kevin, would you accept to pick the series in your tree?
>>
>
> It's too late for v4.7 for us too, please pick up the arch/arm patches
> in your normal stm32 tree and send them to us along with any other changes
> you may have, and resend the driver by itself to netdev time after the
> merge window.
>
> The binding document can go either way, with the dts changes or with
> the driver. I see no dependencies between the patches, so we just need
> to land them all in v4.8.

Ok, this is good for me.
No problem to wait for v4.8.
I will take the arch/arm patches, and David the drivers/net/ ones once
v4.7-rc1 is out.

Thanks for your time,
Maxime

Re: [RESEND PATCH v6 0/6] Add Ethernet support on STM32F429

[Maxime Coquelin]

2016-05-17 18:25 GMT+02:00 David Miller <da...@davemloft.net>:
> From: Maxime Coquelin <mcoquelin.st...@gmail.com>
> Date: Tue, 17 May 2016 11:20:16 +0200
>
>> Hi David,
>>
>> 2016-05-09 21:06 GMT+02:00 David Miller <da...@davemloft.net>:
>>> From: Alexandre TORGUE <alexandre.tor...@gmail.com>
>>> Date: Mon,  9 May 2016 12:31:33 +0200
>>>
>>>> STM32F429 Chip embeds a Synopsys 3.50a MAC IP.
>>>> This series:
>>>>  -enhance current stmmac driver to control it (code already
>>>> available) and adds basic glue for STM32F429 chip.
>>>>  -Enable basic Net config in kernel.
>>>
>>> I assume this will go via the ARM tree, for the networking bits:
>> I would expect patches 1, 2 & 3 to got via your tree, to avoid conflicts.
>
> I don't think putting them all via the ARM tree is going to create much
> in the way of conflicts, and right now during the merge window offloading
> that work from me would really help my backlog a lot.

Ok, I understand this is not the best time to pick it.

Arnd, Olof & Kevin, would you accept to pick the series in your tree?

Regards,
Maxime

Re: [RESEND PATCH v6 0/6] Add Ethernet support on STM32F429

[Maxime Coquelin]

2016-05-17 18:25 GMT+02:00 David Miller :
> From: Maxime Coquelin 
> Date: Tue, 17 May 2016 11:20:16 +0200
>
>> Hi David,
>>
>> 2016-05-09 21:06 GMT+02:00 David Miller :
>>> From: Alexandre TORGUE 
>>> Date: Mon,  9 May 2016 12:31:33 +0200
>>>
>>>> STM32F429 Chip embeds a Synopsys 3.50a MAC IP.
>>>> This series:
>>>>  -enhance current stmmac driver to control it (code already
>>>> available) and adds basic glue for STM32F429 chip.
>>>>  -Enable basic Net config in kernel.
>>>
>>> I assume this will go via the ARM tree, for the networking bits:
>> I would expect patches 1, 2 & 3 to got via your tree, to avoid conflicts.
>
> I don't think putting them all via the ARM tree is going to create much
> in the way of conflicts, and right now during the merge window offloading
> that work from me would really help my backlog a lot.

Ok, I understand this is not the best time to pick it.

Arnd, Olof & Kevin, would you accept to pick the series in your tree?

Regards,
Maxime

Re: [PATCH 2/5] i2c: Add STM32F4 I2C driver

[Maxime Coquelin]

Hi Cedric,

2016-05-11 17:36 GMT+02:00 M'boumba Cedric Madianga :
> This patch adds support for the STM32F4 I2C controller.
>
> Signed-off-by: M'boumba Cedric Madianga 
> ---
>  drivers/i2c/busses/Kconfig   |  10 +
>  drivers/i2c/busses/Makefile  |   1 +
>  drivers/i2c/busses/i2c-stm32f4.c | 872 
> +++
>  3 files changed, 883 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-stm32f4.c
>
...
> diff --git a/drivers/i2c/busses/i2c-stm32f4.c 
> b/drivers/i2c/busses/i2c-stm32f4.c
> new file mode 100644
> index 000..4692213
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-stm32f4.c
...
> +enum stm32f4_i2c_speed {
> +   STM32F4_I2C_SPEED_STANDARD, /* 100 kHz */
> +   STM32F4_I2C_SPEED_FAST, /* 400 kHz */
> +   STM32F4_I2C_SPEED_END,
> +};
> +
> +/**
> + * struct stm32f4_i2c_timings - per-Mode tuning parameters
> + * @rate: I2C bus rate
> + * @duty: Fast mode duty cycle
> + */
> +struct stm32f4_i2c_timings {
> +   u32 rate;
> +   u32 duty;
> +};
...
> +/**
> + * struct stm32f4_i2c_dev - private data of the controller
> + * @adap: I2C adapter for this controller
> + * @dev: device for this controller
> + * @base: virtual memory area
> + * @complete: completion of I2C message
> + * @irq_event: interrupt event line for the controller
> + * @irq_error: interrupt error line for the controller
> + * @clk: hw i2c clock
> + * speed: I2C clock frequency of the controller. Standard or Fast only 
> supported
> + * @client: I2C transfer information
> + * @busy: I2C transfer on-going
> + * @rst: I2C reset line
> + */
> +struct stm32f4_i2c_dev {
> +   struct i2c_adapter  adap;
> +   struct device   *dev;
> +   void __iomem*base;
> +   struct completion   complete;
> +   int irq_event;
> +   int irq_error;
> +   struct clk  *clk;
> +   int speed;
> +   struct stm32f4_i2c_client   client;
> +   boolbusy;
> +   struct reset_control*rst;
> +};
> +
> +static struct stm32f4_i2c_timings i2c_timings[] = {
> +   [STM32F4_I2C_SPEED_STANDARD] = {
> +   .rate   = 10,
> +   .duty   = 0,
> +   },
> +   [STM32F4_I2C_SPEED_FAST] = {
> +   .rate   = 40,
> +   .duty   = 0,
> +   },
> +};
Is this table really needed?
As duty value seems to always be 0, couldn't you just store the rate
in the speed field?


> +static void stm32f4_i2c_set_rise_time(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +   u32 trise, freq, cr2;
> +
> +   cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
> +   freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
> +
> +   trise = readl_relaxed(i2c_dev->base + STM32F4_I2C_TRISE);
> +   trise &= ~STM32F4_I2C_TRISE_TRISE_MASK;
> +
> +   if (i2c_dev->speed == STM32F4_I2C_SPEED_STANDARD)
> +   trise |= STM32F4_I2C_TRISE_TRISE((freq + 1));
> +   else
> +   trise |= STM32F4_I2C_TRISE_TRISEfreq * 300) / 1000) + 1));
Or if you keep the timings struct, maybe the rise time should be part of it.
Doing, that, you will avoid the if/else

> +
> +   writel_relaxed(trise, i2c_dev->base + STM32F4_I2C_TRISE);
> +}
> +
> +static void stm32f4_i2c_set_speed_mode(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +   struct stm32f4_i2c_timings *t = _timings[i2c_dev->speed];
> +   u32 ccr, val, clk_rate;
> +
> +   ccr = readl_relaxed(i2c_dev->base + STM32F4_I2C_CCR);
> +   ccr &= ~(STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY |
> +STM32F4_I2C_CCR_CCR_MASK);
> +
> +   clk_rate = clk_get_rate(i2c_dev->clk);
> +
> +   switch (i2c_dev->speed) {
> +   case STM32F4_I2C_SPEED_STANDARD:
> +   val = clk_rate / t->rate * 2;
> +   if (val < STM32F4_I2C_MIN_CCR)
> +   ccr |= STM32F4_I2C_CCR_CCR(STM32F4_I2C_MIN_CCR);
> +   else
> +   ccr |= STM32F4_I2C_CCR_CCR(val);
> +   break;
> +   case STM32F4_I2C_SPEED_FAST:
> +   ccr |= STM32F4_I2C_CCR_FS;
> +   if (t->duty) {
> +   ccr |= STM32F4_I2C_CCR_DUTY;
> +   ccr |= STM32F4_I2C_CCR_CCR(clk_rate / t->rate * 25);
> +   } else {
> +   ccr |= STM32F4_I2C_CCR_CCR(clk_rate / t->rate * 3);
> +   }
Is it really useful since duty seems to always be 0?

> +   break;
> +   default:
> +   dev_err(i2c_dev->dev, "I2C speed mode not supported\n");
> +   }
> +
> +   writel_relaxed(ccr, i2c_dev->base + STM32F4_I2C_CCR);
> +}
> +
> +static void stm32f4_i2c_set_filter(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +   u32 

Re: [PATCH 2/5] i2c: Add STM32F4 I2C driver

[Maxime Coquelin]

Hi Cedric,

2016-05-11 17:36 GMT+02:00 M'boumba Cedric Madianga :
> This patch adds support for the STM32F4 I2C controller.
>
> Signed-off-by: M'boumba Cedric Madianga 
> ---
>  drivers/i2c/busses/Kconfig   |  10 +
>  drivers/i2c/busses/Makefile  |   1 +
>  drivers/i2c/busses/i2c-stm32f4.c | 872 
> +++
>  3 files changed, 883 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-stm32f4.c
>
...
> diff --git a/drivers/i2c/busses/i2c-stm32f4.c 
> b/drivers/i2c/busses/i2c-stm32f4.c
> new file mode 100644
> index 000..4692213
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-stm32f4.c
...
> +enum stm32f4_i2c_speed {
> +   STM32F4_I2C_SPEED_STANDARD, /* 100 kHz */
> +   STM32F4_I2C_SPEED_FAST, /* 400 kHz */
> +   STM32F4_I2C_SPEED_END,
> +};
> +
> +/**
> + * struct stm32f4_i2c_timings - per-Mode tuning parameters
> + * @rate: I2C bus rate
> + * @duty: Fast mode duty cycle
> + */
> +struct stm32f4_i2c_timings {
> +   u32 rate;
> +   u32 duty;
> +};
...
> +/**
> + * struct stm32f4_i2c_dev - private data of the controller
> + * @adap: I2C adapter for this controller
> + * @dev: device for this controller
> + * @base: virtual memory area
> + * @complete: completion of I2C message
> + * @irq_event: interrupt event line for the controller
> + * @irq_error: interrupt error line for the controller
> + * @clk: hw i2c clock
> + * speed: I2C clock frequency of the controller. Standard or Fast only 
> supported
> + * @client: I2C transfer information
> + * @busy: I2C transfer on-going
> + * @rst: I2C reset line
> + */
> +struct stm32f4_i2c_dev {
> +   struct i2c_adapter  adap;
> +   struct device   *dev;
> +   void __iomem*base;
> +   struct completion   complete;
> +   int irq_event;
> +   int irq_error;
> +   struct clk  *clk;
> +   int speed;
> +   struct stm32f4_i2c_client   client;
> +   boolbusy;
> +   struct reset_control*rst;
> +};
> +
> +static struct stm32f4_i2c_timings i2c_timings[] = {
> +   [STM32F4_I2C_SPEED_STANDARD] = {
> +   .rate   = 10,
> +   .duty   = 0,
> +   },
> +   [STM32F4_I2C_SPEED_FAST] = {
> +   .rate   = 40,
> +   .duty   = 0,
> +   },
> +};
Is this table really needed?
As duty value seems to always be 0, couldn't you just store the rate
in the speed field?


> +static void stm32f4_i2c_set_rise_time(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +   u32 trise, freq, cr2;
> +
> +   cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
> +   freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
> +
> +   trise = readl_relaxed(i2c_dev->base + STM32F4_I2C_TRISE);
> +   trise &= ~STM32F4_I2C_TRISE_TRISE_MASK;
> +
> +   if (i2c_dev->speed == STM32F4_I2C_SPEED_STANDARD)
> +   trise |= STM32F4_I2C_TRISE_TRISE((freq + 1));
> +   else
> +   trise |= STM32F4_I2C_TRISE_TRISEfreq * 300) / 1000) + 1));
Or if you keep the timings struct, maybe the rise time should be part of it.
Doing, that, you will avoid the if/else

> +
> +   writel_relaxed(trise, i2c_dev->base + STM32F4_I2C_TRISE);
> +}
> +
> +static void stm32f4_i2c_set_speed_mode(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +   struct stm32f4_i2c_timings *t = _timings[i2c_dev->speed];
> +   u32 ccr, val, clk_rate;
> +
> +   ccr = readl_relaxed(i2c_dev->base + STM32F4_I2C_CCR);
> +   ccr &= ~(STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY |
> +STM32F4_I2C_CCR_CCR_MASK);
> +
> +   clk_rate = clk_get_rate(i2c_dev->clk);
> +
> +   switch (i2c_dev->speed) {
> +   case STM32F4_I2C_SPEED_STANDARD:
> +   val = clk_rate / t->rate * 2;
> +   if (val < STM32F4_I2C_MIN_CCR)
> +   ccr |= STM32F4_I2C_CCR_CCR(STM32F4_I2C_MIN_CCR);
> +   else
> +   ccr |= STM32F4_I2C_CCR_CCR(val);
> +   break;
> +   case STM32F4_I2C_SPEED_FAST:
> +   ccr |= STM32F4_I2C_CCR_FS;
> +   if (t->duty) {
> +   ccr |= STM32F4_I2C_CCR_DUTY;
> +   ccr |= STM32F4_I2C_CCR_CCR(clk_rate / t->rate * 25);
> +   } else {
> +   ccr |= STM32F4_I2C_CCR_CCR(clk_rate / t->rate * 3);
> +   }
Is it really useful since duty seems to always be 0?

> +   break;
> +   default:
> +   dev_err(i2c_dev->dev, "I2C speed mode not supported\n");
> +   }
> +
> +   writel_relaxed(ccr, i2c_dev->base + STM32F4_I2C_CCR);
> +}
> +
> +static void stm32f4_i2c_set_filter(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +   u32 filter;
> +
> +   /* Enable analog noise filter 

Re: [RESEND PATCH v6 0/6] Add Ethernet support on STM32F429

[Maxime Coquelin]

Hi David,

2016-05-09 21:06 GMT+02:00 David Miller :
> From: Alexandre TORGUE 
> Date: Mon,  9 May 2016 12:31:33 +0200
>
>> STM32F429 Chip embeds a Synopsys 3.50a MAC IP.
>> This series:
>>  -enhance current stmmac driver to control it (code already
>> available) and adds basic glue for STM32F429 chip.
>>  -Enable basic Net config in kernel.
>
> I assume this will go via the ARM tree, for the networking bits:
I would expect patches 1, 2 & 3 to got via your tree, to avoid conflicts.

> Acked-by: David S. Miller 

Thanks,
Maxime

Re: [RESEND PATCH v6 0/6] Add Ethernet support on STM32F429

[Maxime Coquelin]

Hi David,

2016-05-09 21:06 GMT+02:00 David Miller :
> From: Alexandre TORGUE 
> Date: Mon,  9 May 2016 12:31:33 +0200
>
>> STM32F429 Chip embeds a Synopsys 3.50a MAC IP.
>> This series:
>>  -enhance current stmmac driver to control it (code already
>> available) and adds basic glue for STM32F429 chip.
>>  -Enable basic Net config in kernel.
>
> I assume this will go via the ARM tree, for the networking bits:
I would expect patches 1, 2 & 3 to got via your tree, to avoid conflicts.

> Acked-by: David S. Miller 

Thanks,
Maxime

Re: [PATCH v2 6/9] pinctrl: Add IRQ support to STM32 gpios

[Maxime Coquelin]

2016-04-30 13:32 GMT+02:00 Linus Walleij <linus.wall...@linaro.org>:
> On Fri, Apr 29, 2016 at 1:19 PM, Maxime Coquelin
> <mcoquelin.st...@gmail.com> wrote:
>> It looks like this: http://pastebin.com/raw/cs2WiNKZ
>> You can directly check section 12.2.5 of the stm32f429 reference manual:
>> http://www2.st.com/resource/en/reference_manual/dm00031020.pdf
>
> Nice ASCII art, include that into the commit message :)

I will! :)

>
>> For example, we can imagine a board where gpioa0 is used SD's card detect,
>> and gpiob0 used to control a led.
>>
>> If we set the mux at request time, gpiob0 request may overwrite the mux
>> configuration set by gpioa0, whereas it is not used as interrupt.
>>
>> That is the reason why I did it in .to_irq().
>
> Well now I am even *more* convinced that this should not happen in
> .to_irq(). .to_irq() should not do *anything*.
>
> This needs to happen as part of the irqchip setting up the actual
> interrupt.
>
> And it seems the problem is that this driver does *not* define its
> own irqchip, but it *should*.
>
> What you want to do is implement an hierarchical irqdomain in your
> irqchip, which is what other drivers of this type are doing, see:
> drivers/gpio/gpio-xgene-sb.c
> irq_domain_create_hierarchy() is your friend.
>
>>> It should *also* be done in the set-up path for the irqchip
>>> side, if that line is used without any interaction with the
>>> gpio side of things.
>>
>> Actually, a line is either used by a GPIO, (exclusive) or another purpose.
>> In the case of a GPIO line, I think we should always go through the gpio.
>
> This is a textbook example of a hierarchichal irq domain I think.
>
>>> The point is that each IRQ that ever get used
>>> has a 1-to-1 relationship to a certain GPIO line, and if that
>>> relationship cannot be resolved from the irqchip side,
>>> something is wrong. The irqchip needs to enable the
>>> GPIO line it is backing to recieve interrupts without any
>>> requirements that .to_irq() have been called first.
>>
>> Actually, this is not a 1:1 relationship, as for example, exti0 can be 
>> connected
>> to either gpioa0, or gpiob0,..., or gpiok0.
>
> That is what hierarchical irqdomain is for.
>
> You should expose an irqchip from the gpio driver, that give you
> unique irq translations for *all* GPIO lines.
>
> Then, at runtime, if the hierarchical irqdomain cannot map
> (i.e. mux) the interrupt onto one of the available lines,
> you will get an error.

Thanks a lot for this valuable feedback.
I will have a look at hierachical irq domains, and hope to come back this week
with an implementation.

Regards,
Maxime

Re: [PATCH v2 6/9] pinctrl: Add IRQ support to STM32 gpios

[Maxime Coquelin]

2016-04-30 13:32 GMT+02:00 Linus Walleij :
> On Fri, Apr 29, 2016 at 1:19 PM, Maxime Coquelin
>  wrote:
>> It looks like this: http://pastebin.com/raw/cs2WiNKZ
>> You can directly check section 12.2.5 of the stm32f429 reference manual:
>> http://www2.st.com/resource/en/reference_manual/dm00031020.pdf
>
> Nice ASCII art, include that into the commit message :)

I will! :)

>
>> For example, we can imagine a board where gpioa0 is used SD's card detect,
>> and gpiob0 used to control a led.
>>
>> If we set the mux at request time, gpiob0 request may overwrite the mux
>> configuration set by gpioa0, whereas it is not used as interrupt.
>>
>> That is the reason why I did it in .to_irq().
>
> Well now I am even *more* convinced that this should not happen in
> .to_irq(). .to_irq() should not do *anything*.
>
> This needs to happen as part of the irqchip setting up the actual
> interrupt.
>
> And it seems the problem is that this driver does *not* define its
> own irqchip, but it *should*.
>
> What you want to do is implement an hierarchical irqdomain in your
> irqchip, which is what other drivers of this type are doing, see:
> drivers/gpio/gpio-xgene-sb.c
> irq_domain_create_hierarchy() is your friend.
>
>>> It should *also* be done in the set-up path for the irqchip
>>> side, if that line is used without any interaction with the
>>> gpio side of things.
>>
>> Actually, a line is either used by a GPIO, (exclusive) or another purpose.
>> In the case of a GPIO line, I think we should always go through the gpio.
>
> This is a textbook example of a hierarchichal irq domain I think.
>
>>> The point is that each IRQ that ever get used
>>> has a 1-to-1 relationship to a certain GPIO line, and if that
>>> relationship cannot be resolved from the irqchip side,
>>> something is wrong. The irqchip needs to enable the
>>> GPIO line it is backing to recieve interrupts without any
>>> requirements that .to_irq() have been called first.
>>
>> Actually, this is not a 1:1 relationship, as for example, exti0 can be 
>> connected
>> to either gpioa0, or gpiob0,..., or gpiok0.
>
> That is what hierarchical irqdomain is for.
>
> You should expose an irqchip from the gpio driver, that give you
> unique irq translations for *all* GPIO lines.
>
> Then, at runtime, if the hierarchical irqdomain cannot map
> (i.e. mux) the interrupt onto one of the available lines,
> you will get an error.

Thanks a lot for this valuable feedback.
I will have a look at hierachical irq domains, and hope to come back this week
with an implementation.

Regards,
Maxime

Re: [PATCH] i2c: st: Implement i2c_bus_recovery_info callbacks

[Maxime Coquelin]

On 04/28/2016 04:57 PM, Wolfram Sang wrote:

The trick is to switch to SPI mode, 9 bits words and write a 0,
so that 9 clock pulses are generated.

Heh. As long as it works :) But as you said, it really needs a comment.


:)
I didn't faced the problem myself, but it looks good with an oscilloscope,
and a customer reported it to work.

Peter, will you resend with adding the explanations I provided as comment?

Thanks in advance,
Maxime

Re: [PATCH] i2c: st: Implement i2c_bus_recovery_info callbacks

[Maxime Coquelin]

On 04/28/2016 04:57 PM, Wolfram Sang wrote:

The trick is to switch to SPI mode, 9 bits words and write a 0,
so that 9 clock pulses are generated.

Heh. As long as it works :) But as you said, it really needs a comment.


:)
I didn't faced the problem myself, but it looks good with an oscilloscope,
and a customer reported it to work.

Peter, will you resend with adding the explanations I provided as comment?

Thanks in advance,
Maxime

Re: [PATCH v6 2/6] Documentation: Bindings: Add STM32 DWMAC glue

[Maxime Coquelin]

2016-04-28 22:59 GMT+02:00 Rob Herring :
> On Mon, Apr 25, 2016 at 01:53:58PM +0200, Alexandre TORGUE wrote:
>> Signed-off-by: Alexandre TORGUE 
>
> Acked-by: Rob Herring 

Thanks Rob!

Arnd, I only have patches 4, 5 and 6 of this series for stm32 (2 DT,
one defconfig) for v4.7.
Should I send a pull request, or I can send the patches directly to
a...@kernel.org so that
you apply them directly?

Thanks in advance,
Maxime

Re: [PATCH v6 2/6] Documentation: Bindings: Add STM32 DWMAC glue

[Maxime Coquelin]

2016-04-28 22:59 GMT+02:00 Rob Herring :
> On Mon, Apr 25, 2016 at 01:53:58PM +0200, Alexandre TORGUE wrote:
>> Signed-off-by: Alexandre TORGUE 
>
> Acked-by: Rob Herring 

Thanks Rob!

Arnd, I only have patches 4, 5 and 6 of this series for stm32 (2 DT,
one defconfig) for v4.7.
Should I send a pull request, or I can send the patches directly to
a...@kernel.org so that
you apply them directly?

Thanks in advance,
Maxime

Re: [PATCH v2 6/9] pinctrl: Add IRQ support to STM32 gpios

[Maxime Coquelin]

2016-04-29 10:53 GMT+02:00 Linus Walleij <linus.wall...@linaro.org>:
> On Tue, Apr 19, 2016 at 11:04 AM, Maxime Coquelin
> <mcoquelin.st...@gmail.com> wrote:
>> 2016-04-08 11:43 GMT+02:00 Linus Walleij <linus.wall...@linaro.org>:
>>> On Thu, Mar 31, 2016 at 5:09 PM, Maxime Coquelin
>>> <mcoquelin.st...@gmail.com> wrote:
>>>
>>>> +static int stm32_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>>>> +{
>>>> +   struct stm32_pinctrl *pctl = dev_get_drvdata(chip->parent);
>>>> +   struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
>>>> +   unsigned int irq;
>>>> +
>>>> +   regmap_field_write(pctl->irqmux[offset], bank->range.id);
>>>
>>> No. You must implement the irqchip and GPIO controllers to
>>> be orthogonal, doing things like this creates a semantic that
>>> assumes .to_irq() is always called before using the IRQ and
>>> that is not guaranteed at all. A consumer may very well
>>> use an interrupt right off the irqchip without this being called
>>> first. All this function should do is translate a number. No
>>> other semantics.
>>>
>>> This needs to be done from the irqchip (sorry).
>>
>> Actually, the register written here is not part of the irqchip.
>> It is a system config register that is only used when using a GPIO as
>> external interrupt.
>> Its aim is to mux the GPIO bank on a line.
>
> Then it should be done in .request() for the GPIO, not in
> .to_irq().
Problem is that at request time, we don't know whether the GPIO is to
be used as an interrupt or not.

I think I may have not been clear enough in the HW architecture description.
Indeed, I used the term "mux", but should instead use the term "selector".

The idea is that between the GPIO controllers and the EXTI controller,
there is one selector for each line number, to select the controller used as
interrupt source.

For example, there is a selector on line 0 to select between gpioa0, gpiob0,
gpioc0,.., gpiok0, which one is connected to exti0.

It means there is a HW restriction that makes impossible to use both GPIOA0
or GPIOB0 as interrupts at the same time.

It looks like this: http://pastebin.com/raw/cs2WiNKZ
You can directly check section 12.2.5 of the stm32f429 reference manual:
http://www2.st.com/resource/en/reference_manual/dm00031020.pdf

For example, we can imagine a board where gpioa0 is used SD's card detect,
and gpiob0 used to control a led.

If we set the mux at request time, gpiob0 request may overwrite the mux
configuration set by gpioa0, whereas it is not used as interrupt.

That is the reason why I did it in .to_irq().

>
> It should *also* be done in the set-up path for the irqchip
> side, if that line is used without any interaction with the
> gpio side of things.
Actually, a line is either used by a GPIO, (exclusive) or another purpose.
In the case of a GPIO line, I think we should always go through the gpio.

>
>> For example on line 1, it can be muxed to select either gpioa1,
>> gpiob1, gpioc1, ...
>> How could I do it in the irqchip that has no gpio knowledge?
>
> I don't understand this.
>
> We are discussing an irqchip that is tightly coupled with
> a gpiochip. Usually d->hwirq is the same as the GPIO offset
> but that varies.
>
> The point is that each IRQ that ever get used
> has a 1-to-1 relationship to a certain GPIO line, and if that
> relationship cannot be resolved from the irqchip side,
> something is wrong. The irqchip needs to enable the
> GPIO line it is backing to recieve interrupts without any
> requirements that .to_irq() have been called first.

Actually, this is not a 1:1 relationship, as for example, exti0 can be connected
to either gpioa0, or gpiob0,..., or gpiok0.

So for the exti entries that are connected to gpios, I don't see how
we can reference it
from the exti controller,
>
> If to_irq() does something else than translate to an IRQ
> something is wrong.

Ok, so maybe we need an intermediate layer between gpio and exti to manage the
selectors sysconf?
I wiil have a look in that direction, but I think it could be over-engineered.

Thanks!
Maxime

Re: [PATCH v2 6/9] pinctrl: Add IRQ support to STM32 gpios

[Maxime Coquelin]

2016-04-29 10:53 GMT+02:00 Linus Walleij :
> On Tue, Apr 19, 2016 at 11:04 AM, Maxime Coquelin
>  wrote:
>> 2016-04-08 11:43 GMT+02:00 Linus Walleij :
>>> On Thu, Mar 31, 2016 at 5:09 PM, Maxime Coquelin
>>>  wrote:
>>>
>>>> +static int stm32_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>>>> +{
>>>> +   struct stm32_pinctrl *pctl = dev_get_drvdata(chip->parent);
>>>> +   struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
>>>> +   unsigned int irq;
>>>> +
>>>> +   regmap_field_write(pctl->irqmux[offset], bank->range.id);
>>>
>>> No. You must implement the irqchip and GPIO controllers to
>>> be orthogonal, doing things like this creates a semantic that
>>> assumes .to_irq() is always called before using the IRQ and
>>> that is not guaranteed at all. A consumer may very well
>>> use an interrupt right off the irqchip without this being called
>>> first. All this function should do is translate a number. No
>>> other semantics.
>>>
>>> This needs to be done from the irqchip (sorry).
>>
>> Actually, the register written here is not part of the irqchip.
>> It is a system config register that is only used when using a GPIO as
>> external interrupt.
>> Its aim is to mux the GPIO bank on a line.
>
> Then it should be done in .request() for the GPIO, not in
> .to_irq().
Problem is that at request time, we don't know whether the GPIO is to
be used as an interrupt or not.

I think I may have not been clear enough in the HW architecture description.
Indeed, I used the term "mux", but should instead use the term "selector".

The idea is that between the GPIO controllers and the EXTI controller,
there is one selector for each line number, to select the controller used as
interrupt source.

For example, there is a selector on line 0 to select between gpioa0, gpiob0,
gpioc0,.., gpiok0, which one is connected to exti0.

It means there is a HW restriction that makes impossible to use both GPIOA0
or GPIOB0 as interrupts at the same time.

It looks like this: http://pastebin.com/raw/cs2WiNKZ
You can directly check section 12.2.5 of the stm32f429 reference manual:
http://www2.st.com/resource/en/reference_manual/dm00031020.pdf

For example, we can imagine a board where gpioa0 is used SD's card detect,
and gpiob0 used to control a led.

If we set the mux at request time, gpiob0 request may overwrite the mux
configuration set by gpioa0, whereas it is not used as interrupt.

That is the reason why I did it in .to_irq().

>
> It should *also* be done in the set-up path for the irqchip
> side, if that line is used without any interaction with the
> gpio side of things.
Actually, a line is either used by a GPIO, (exclusive) or another purpose.
In the case of a GPIO line, I think we should always go through the gpio.

>
>> For example on line 1, it can be muxed to select either gpioa1,
>> gpiob1, gpioc1, ...
>> How could I do it in the irqchip that has no gpio knowledge?
>
> I don't understand this.
>
> We are discussing an irqchip that is tightly coupled with
> a gpiochip. Usually d->hwirq is the same as the GPIO offset
> but that varies.
>
> The point is that each IRQ that ever get used
> has a 1-to-1 relationship to a certain GPIO line, and if that
> relationship cannot be resolved from the irqchip side,
> something is wrong. The irqchip needs to enable the
> GPIO line it is backing to recieve interrupts without any
> requirements that .to_irq() have been called first.

Actually, this is not a 1:1 relationship, as for example, exti0 can be connected
to either gpioa0, or gpiob0,..., or gpiok0.

So for the exti entries that are connected to gpios, I don't see how
we can reference it
from the exti controller,
>
> If to_irq() does something else than translate to an IRQ
> something is wrong.

Ok, so maybe we need an intermediate layer between gpio and exti to manage the
selectors sysconf?
I wiil have a look in that direction, but I think it could be over-engineered.

Thanks!
Maxime

Re: [PATCH] i2c: st: Implement i2c_bus_recovery_info callbacks

[Maxime Coquelin]

Hi Wolfram,

On 04/24/2016 11:10 PM, Wolfram Sang wrote:

+/*
+ * i2c bus recovery routines
+ * get_scl and set_scl must be defined to avoid the recover_bus field of
+ * i2c_bus_recovery_info to be overriden with NULL during the
+ * i2c_add_adapter call
+ */

Oh, that shouldn't be like this. Can you try this patch and remove the
empty functions please?

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 4979728f7fb2de..604936955807e5 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1595,10 +1595,12 @@ static int i2c_register_adapter(struct i2c_adapter 
*adap)
  
  			bri->get_scl = get_scl_gpio_value;

bri->set_scl = set_scl_gpio_value;
-   } else if (!bri->set_scl || !bri->get_scl) {
+   } else if (bri->recover_bus == i2c_generic_scl_recovery) {
/* Generic SCL recovery */
-   dev_err(>dev, "No {get|set}_gpio() found, not using 
recovery\n");
-   adap->bus_recovery_info = NULL;
+   if (!bri->set_scl || !bri->get_scl) {
+   dev_err(>dev, "No {get|set}_scl() found, not 
using recovery\n");
+   adap->bus_recovery_info = NULL;
+   }
}
}
  


+static int st_i2c_recover_bus(struct i2c_adapter *i2c_adap)
+{

Can you describe what the function does? It is not clear to me that it
generates 9 scl pulses.

I agree, it would need some comments.
This IP is dual-role, it can do either SPI or I2C.
The trick is to switch to SPI mode, 9 bits words and write a 0,
so that 9 clock pulses are generated.

This is easier to manage than switching to GPIO mode,
as we don't need to provide the gpio handles in DT, and no need to 
put/get the pinctrl handle.


Regards,
Maxime

Re: [PATCH] i2c: st: Implement i2c_bus_recovery_info callbacks

[Maxime Coquelin]

Hi Wolfram,

On 04/24/2016 11:10 PM, Wolfram Sang wrote:

+/*
+ * i2c bus recovery routines
+ * get_scl and set_scl must be defined to avoid the recover_bus field of
+ * i2c_bus_recovery_info to be overriden with NULL during the
+ * i2c_add_adapter call
+ */

Oh, that shouldn't be like this. Can you try this patch and remove the
empty functions please?

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 4979728f7fb2de..604936955807e5 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1595,10 +1595,12 @@ static int i2c_register_adapter(struct i2c_adapter 
*adap)
  
  			bri->get_scl = get_scl_gpio_value;

bri->set_scl = set_scl_gpio_value;
-   } else if (!bri->set_scl || !bri->get_scl) {
+   } else if (bri->recover_bus == i2c_generic_scl_recovery) {
/* Generic SCL recovery */
-   dev_err(>dev, "No {get|set}_gpio() found, not using 
recovery\n");
-   adap->bus_recovery_info = NULL;
+   if (!bri->set_scl || !bri->get_scl) {
+   dev_err(>dev, "No {get|set}_scl() found, not 
using recovery\n");
+   adap->bus_recovery_info = NULL;
+   }
}
}
  


+static int st_i2c_recover_bus(struct i2c_adapter *i2c_adap)
+{

Can you describe what the function does? It is not clear to me that it
generates 9 scl pulses.

I agree, it would need some comments.
This IP is dual-role, it can do either SPI or I2C.
The trick is to switch to SPI mode, 9 bits words and write a 0,
so that 9 clock pulses are generated.

This is easier to manage than switching to GPIO mode,
as we don't need to provide the gpio handles in DT, and no need to 
put/get the pinctrl handle.


Regards,
Maxime

Re: [PATCH v6 6/6] ARM: dts: stm32f429: Update Ethernet node on Eval board

[Maxime Coquelin]

2016-04-25 13:54 GMT+02:00 Alexandre TORGUE <alexandre.tor...@gmail.com>:
> Update new pinctrl phandle name and use new node name.
>
> Signed-off-by: Alexandre TORGUE <alexandre.tor...@gmail.com>
>
> diff --git a/arch/arm/boot/dts/stm32429i-eval.dts 
> b/arch/arm/boot/dts/stm32429i-eval.dts

Acked-by: Maxime Coquelin <mcoquelin.st...@gmail.com>

Thanks!
Maxime

Re: [PATCH v6 6/6] ARM: dts: stm32f429: Update Ethernet node on Eval board

[Maxime Coquelin]

2016-04-25 13:54 GMT+02:00 Alexandre TORGUE :
> Update new pinctrl phandle name and use new node name.
>
> Signed-off-by: Alexandre TORGUE 
>
> diff --git a/arch/arm/boot/dts/stm32429i-eval.dts 
> b/arch/arm/boot/dts/stm32429i-eval.dts

Acked-by: Maxime Coquelin 

Thanks!
Maxime

Re: [PATCH v6 5/6] ARM: dts: stm32f429: Align Ethernet node with new bindings properties

[Maxime Coquelin]

Hi Alex,

2016-04-25 13:54 GMT+02:00 Alexandre TORGUE :
> This patch aligns clocks names and node reference according to new
> stm32-dwmac glue binding. It also renames Ethernet pinctrl phandle
> (indeed there is no need to add 0 as Ethernet instance as there is only
> one IP in SOC).
>
> Signed-off-by: Alexandre TORGUE 
>
> diff --git a/arch/arm/boot/dts/stm32f429.dtsi 
> b/arch/arm/boot/dts/stm32f429.dtsi
> index 35df462..5995998 100644
> --- a/arch/arm/boot/dts/stm32f429.dtsi
> +++ b/arch/arm/boot/dts/stm32f429.dtsi
> @@ -304,7 +304,7 @@
> };
> };
>
> -   ethernet0_mii: mii@0 {
> +   ethernet_mii: mii@0 {
> pins {
> pinmux = 
> ,
>  
> ,
> @@ -363,13 +363,13 @@
> st,mem2mem;
> };
>
> -   ethernet0: dwmac@40028000 {
> +   mac: ethernet@40028000 {
> compatible = "st,stm32-dwmac", "snps,dwmac-3.50a";
> reg = <0x40028000 0x8000>;
> reg-names = "stmmaceth";
> interrupts = <61>, <62>;
> interrupt-names = "macirq", "eth_wake_irq";
> -   clock-names = "stmmaceth", "tx-clk", "rx-clk";
> +   clock-names = "stmmaceth", "mac-clk-tx", "mac-clk-rx";

It looks good to me, but I will wait for Rob's ack on the bindings
documentation patch before applying it.

Regards,
Maxime

Re: [PATCH v6 5/6] ARM: dts: stm32f429: Align Ethernet node with new bindings properties

[Maxime Coquelin]

Hi Alex,

2016-04-25 13:54 GMT+02:00 Alexandre TORGUE :
> This patch aligns clocks names and node reference according to new
> stm32-dwmac glue binding. It also renames Ethernet pinctrl phandle
> (indeed there is no need to add 0 as Ethernet instance as there is only
> one IP in SOC).
>
> Signed-off-by: Alexandre TORGUE 
>
> diff --git a/arch/arm/boot/dts/stm32f429.dtsi 
> b/arch/arm/boot/dts/stm32f429.dtsi
> index 35df462..5995998 100644
> --- a/arch/arm/boot/dts/stm32f429.dtsi
> +++ b/arch/arm/boot/dts/stm32f429.dtsi
> @@ -304,7 +304,7 @@
> };
> };
>
> -   ethernet0_mii: mii@0 {
> +   ethernet_mii: mii@0 {
> pins {
> pinmux = 
> ,
>  
> ,
> @@ -363,13 +363,13 @@
> st,mem2mem;
> };
>
> -   ethernet0: dwmac@40028000 {
> +   mac: ethernet@40028000 {
> compatible = "st,stm32-dwmac", "snps,dwmac-3.50a";
> reg = <0x40028000 0x8000>;
> reg-names = "stmmaceth";
> interrupts = <61>, <62>;
> interrupt-names = "macirq", "eth_wake_irq";
> -   clock-names = "stmmaceth", "tx-clk", "rx-clk";
> +   clock-names = "stmmaceth", "mac-clk-tx", "mac-clk-rx";

It looks good to me, but I will wait for Rob's ack on the bindings
documentation patch before applying it.

Regards,
Maxime

Re: [PATCH v6 4/6] ARM: STM32: Enable Ethernet in stm32_defconfig

[Maxime Coquelin]

Hi Alex,

2016-04-25 13:54 GMT+02:00 Alexandre TORGUE <alexandre.tor...@gmail.com>:
> Enable basic Ethernet support (IPV4) for stm32 defconfig.
>
> Signed-off-by: Alexandre TORGUE <alexandre.tor...@gmail.com>

Acked-by: Maxime Coquelin <mcoquelin.st...@gmail.com>

Thanks!
Maxime

Re: [PATCH v6 4/6] ARM: STM32: Enable Ethernet in stm32_defconfig

[Maxime Coquelin]

Hi Alex,

2016-04-25 13:54 GMT+02:00 Alexandre TORGUE :
> Enable basic Ethernet support (IPV4) for stm32 defconfig.
>
> Signed-off-by: Alexandre TORGUE 

Acked-by: Maxime Coquelin 

Thanks!
Maxime

Re: [[PATCH v2] 1/8] ARM: dts: STi: STiH407: Provide generic (safe) DVFS configuration

[Maxime Coquelin]

Hi Lee,

Series is applied, thanks for having done the changes.

Note that I had to rework all the commit titles, because of the 
double brackets ("[[PATCH v2] 1/8]").

Something wrong with your script?

Thanks,
Maxime

On 04/21/2016 05:07 PM, Lee Jones wrote:

You'll notice that the voltage cell is populated with 0's.  Voltage
information is very platform specific, even depends on 'cut' and
'substrate' versions.  Thus it is left blank for a generic (safe)
implementation.  If other nodes/properties are provided by the
bootloader, the ST CPUFreq driver will over-ride these generic
values.

Signed-off-by: Lee Jones <lee.jo...@linaro.org>
Signed-off-by: Maxime Coquelin <maxime.coque...@st.com>
---
  arch/arm/boot/dts/stih407-family.dtsi | 14 ++
  1 file changed, 14 insertions(+)

diff --git a/arch/arm/boot/dts/stih407-family.dtsi 
b/arch/arm/boot/dts/stih407-family.dtsi
index 81f8121..9fa1e58 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -22,15 +22,29 @@
device_type = "cpu";
compatible = "arm,cortex-a9";
reg = <0>;
+
/* u-boot puts hpen in SBC dmem at 0xa4 offset */
cpu-release-addr = <0x94100A4>;
+
+/* kHz uV   */
+   operating-points = <150 0
+   120 0
+   80  0
+   50  0>;
};
cpu@1 {
device_type = "cpu";
compatible = "arm,cortex-a9";
reg = <1>;
+
/* u-boot puts hpen in SBC dmem at 0xa4 offset */
cpu-release-addr = <0x94100A4>;
+
+/* kHz uV   */
+   operating-points = <150 0
+   120 0
+   80  0
+   50  0>;
};
};