* [REVIEW][PATCH 02/11] shm/security: Pass kern_ipc_perm not shmid_kernel into the shm security hooks
From: Eric W. Biederman @ 2018-03-23 19:16 UTC (permalink / raw)
To: Linux Containers
Cc: esyr-H+wXaHxf7aLQT0dZR+AlfA, jannh-hpIqsD4AKlfQT0dZR+AlfA,
khlebnikov-XoJtRXgx1JseBXzfvpsJ4g,
linux-api-u79uwXL29TY76Z2rM5mHXA,
serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
prakash.sangappa-QHcLZuEGTsvQT0dZR+AlfA,
linux-security-module-u79uwXL29TY76Z2rM5mHXA, Eric W. Biederman,
luto-DgEjT+Ai2ygdnm+yROfE0A, oleg-H+wXaHxf7aLQT0dZR+AlfA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b, Nagarathnam Muthusamy,
Pavel Emelyanov
In-Reply-To: <87vadmobdw.fsf_-_-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
All of the implementations of security hooks that take shmid_kernel only
access shm_perm the struct kern_ipc_perm member. This means the
dependencies of the shm security hooks can be simplified by passing
the kern_ipc_perm member of shmid_kernel..
Making this change will allow struct shmid_kernel to become private to ipc/shm.c.
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
include/linux/lsm_hooks.h | 10 +++++-----
include/linux/security.h | 21 ++++++++++-----------
ipc/shm.c | 17 +++++++----------
security/security.c | 10 +++++-----
security/selinux/hooks.c | 28 ++++++++++++++--------------
security/smack/smack_lsm.c | 22 +++++++++++-----------
6 files changed, 52 insertions(+), 56 deletions(-)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index e4a94863a88c..cac7a8082c43 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1585,11 +1585,11 @@ union security_list_options {
struct task_struct *target, long type,
int mode);
- int (*shm_alloc_security)(struct shmid_kernel *shp);
- void (*shm_free_security)(struct shmid_kernel *shp);
- int (*shm_associate)(struct shmid_kernel *shp, int shmflg);
- int (*shm_shmctl)(struct shmid_kernel *shp, int cmd);
- int (*shm_shmat)(struct shmid_kernel *shp, char __user *shmaddr,
+ int (*shm_alloc_security)(struct kern_ipc_perm *shp);
+ void (*shm_free_security)(struct kern_ipc_perm *shp);
+ int (*shm_associate)(struct kern_ipc_perm *shp, int shmflg);
+ int (*shm_shmctl)(struct kern_ipc_perm *shp, int cmd);
+ int (*shm_shmat)(struct kern_ipc_perm *shp, char __user *shmaddr,
int shmflg);
int (*sem_alloc_security)(struct kern_ipc_perm *sma);
diff --git a/include/linux/security.h b/include/linux/security.h
index fa7adac4b99a..f390755808ea 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -49,7 +49,6 @@ struct qstr;
struct iattr;
struct fown_struct;
struct file_operations;
-struct shmid_kernel;
struct msg_msg;
struct msg_queue;
struct xattr;
@@ -362,11 +361,11 @@ int security_msg_queue_msgsnd(struct msg_queue *msq,
struct msg_msg *msg, int msqflg);
int security_msg_queue_msgrcv(struct msg_queue *msq, struct msg_msg *msg,
struct task_struct *target, long type, int mode);
-int security_shm_alloc(struct shmid_kernel *shp);
-void security_shm_free(struct shmid_kernel *shp);
-int security_shm_associate(struct shmid_kernel *shp, int shmflg);
-int security_shm_shmctl(struct shmid_kernel *shp, int cmd);
-int security_shm_shmat(struct shmid_kernel *shp, char __user *shmaddr, int shmflg);
+int security_shm_alloc(struct kern_ipc_perm *shp);
+void security_shm_free(struct kern_ipc_perm *shp);
+int security_shm_associate(struct kern_ipc_perm *shp, int shmflg);
+int security_shm_shmctl(struct kern_ipc_perm *shp, int cmd);
+int security_shm_shmat(struct kern_ipc_perm *shp, char __user *shmaddr, int shmflg);
int security_sem_alloc(struct kern_ipc_perm *sma);
void security_sem_free(struct kern_ipc_perm *sma);
int security_sem_associate(struct kern_ipc_perm *sma, int semflg);
@@ -1077,26 +1076,26 @@ static inline int security_msg_queue_msgrcv(struct msg_queue *msq,
return 0;
}
-static inline int security_shm_alloc(struct shmid_kernel *shp)
+static inline int security_shm_alloc(struct kern_ipc_perm *shp)
{
return 0;
}
-static inline void security_shm_free(struct shmid_kernel *shp)
+static inline void security_shm_free(struct kern_ipc_perm *shp)
{ }
-static inline int security_shm_associate(struct shmid_kernel *shp,
+static inline int security_shm_associate(struct kern_ipc_perm *shp,
int shmflg)
{
return 0;
}
-static inline int security_shm_shmctl(struct shmid_kernel *shp, int cmd)
+static inline int security_shm_shmctl(struct kern_ipc_perm *shp, int cmd)
{
return 0;
}
-static inline int security_shm_shmat(struct shmid_kernel *shp,
+static inline int security_shm_shmat(struct kern_ipc_perm *shp,
char __user *shmaddr, int shmflg)
{
return 0;
diff --git a/ipc/shm.c b/ipc/shm.c
index 4643865e9171..387a786e7be1 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -181,7 +181,7 @@ static void shm_rcu_free(struct rcu_head *head)
rcu);
struct shmid_kernel *shp = container_of(ptr, struct shmid_kernel,
shm_perm);
- security_shm_free(shp);
+ security_shm_free(&shp->shm_perm);
kvfree(shp);
}
@@ -554,7 +554,7 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
shp->mlock_user = NULL;
shp->shm_perm.security = NULL;
- error = security_shm_alloc(shp);
+ error = security_shm_alloc(&shp->shm_perm);
if (error) {
kvfree(shp);
return error;
@@ -635,10 +635,7 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
*/
static inline int shm_security(struct kern_ipc_perm *ipcp, int shmflg)
{
- struct shmid_kernel *shp;
-
- shp = container_of(ipcp, struct shmid_kernel, shm_perm);
- return security_shm_associate(shp, shmflg);
+ return security_shm_associate(ipcp, shmflg);
}
/*
@@ -835,7 +832,7 @@ static int shmctl_down(struct ipc_namespace *ns, int shmid, int cmd,
shp = container_of(ipcp, struct shmid_kernel, shm_perm);
- err = security_shm_shmctl(shp, cmd);
+ err = security_shm_shmctl(&shp->shm_perm, cmd);
if (err)
goto out_unlock1;
@@ -934,7 +931,7 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
if (ipcperms(ns, &shp->shm_perm, S_IRUGO))
goto out_unlock;
- err = security_shm_shmctl(shp, cmd);
+ err = security_shm_shmctl(&shp->shm_perm, cmd);
if (err)
goto out_unlock;
@@ -978,7 +975,7 @@ static int shmctl_do_lock(struct ipc_namespace *ns, int shmid, int cmd)
}
audit_ipc_obj(&(shp->shm_perm));
- err = security_shm_shmctl(shp, cmd);
+ err = security_shm_shmctl(&shp->shm_perm, cmd);
if (err)
goto out_unlock1;
@@ -1348,7 +1345,7 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg,
if (ipcperms(ns, &shp->shm_perm, acc_mode))
goto out_unlock;
- err = security_shm_shmat(shp, shmaddr, shmflg);
+ err = security_shm_shmat(&shp->shm_perm, shmaddr, shmflg);
if (err)
goto out_unlock;
diff --git a/security/security.c b/security/security.c
index d3b9aeb6b73b..77b69bd6f234 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1195,27 +1195,27 @@ int security_msg_queue_msgrcv(struct msg_queue *msq, struct msg_msg *msg,
return call_int_hook(msg_queue_msgrcv, 0, msq, msg, target, type, mode);
}
-int security_shm_alloc(struct shmid_kernel *shp)
+int security_shm_alloc(struct kern_ipc_perm *shp)
{
return call_int_hook(shm_alloc_security, 0, shp);
}
-void security_shm_free(struct shmid_kernel *shp)
+void security_shm_free(struct kern_ipc_perm *shp)
{
call_void_hook(shm_free_security, shp);
}
-int security_shm_associate(struct shmid_kernel *shp, int shmflg)
+int security_shm_associate(struct kern_ipc_perm *shp, int shmflg)
{
return call_int_hook(shm_associate, 0, shp, shmflg);
}
-int security_shm_shmctl(struct shmid_kernel *shp, int cmd)
+int security_shm_shmctl(struct kern_ipc_perm *shp, int cmd)
{
return call_int_hook(shm_shmctl, 0, shp, cmd);
}
-int security_shm_shmat(struct shmid_kernel *shp, char __user *shmaddr, int shmflg)
+int security_shm_shmat(struct kern_ipc_perm *shp, char __user *shmaddr, int shmflg)
{
return call_int_hook(shm_shmat, 0, shp, shmaddr, shmflg);
}
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index cce994e9fc0a..14f9e6c08273 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5674,53 +5674,53 @@ static int selinux_msg_queue_msgrcv(struct msg_queue *msq, struct msg_msg *msg,
}
/* Shared Memory security operations */
-static int selinux_shm_alloc_security(struct shmid_kernel *shp)
+static int selinux_shm_alloc_security(struct kern_ipc_perm *shp)
{
struct ipc_security_struct *isec;
struct common_audit_data ad;
u32 sid = current_sid();
int rc;
- rc = ipc_alloc_security(&shp->shm_perm, SECCLASS_SHM);
+ rc = ipc_alloc_security(shp, SECCLASS_SHM);
if (rc)
return rc;
- isec = shp->shm_perm.security;
+ isec = shp->security;
ad.type = LSM_AUDIT_DATA_IPC;
- ad.u.ipc_id = shp->shm_perm.key;
+ ad.u.ipc_id = shp->key;
rc = avc_has_perm(sid, isec->sid, SECCLASS_SHM,
SHM__CREATE, &ad);
if (rc) {
- ipc_free_security(&shp->shm_perm);
+ ipc_free_security(shp);
return rc;
}
return 0;
}
-static void selinux_shm_free_security(struct shmid_kernel *shp)
+static void selinux_shm_free_security(struct kern_ipc_perm *shp)
{
- ipc_free_security(&shp->shm_perm);
+ ipc_free_security(shp);
}
-static int selinux_shm_associate(struct shmid_kernel *shp, int shmflg)
+static int selinux_shm_associate(struct kern_ipc_perm *shp, int shmflg)
{
struct ipc_security_struct *isec;
struct common_audit_data ad;
u32 sid = current_sid();
- isec = shp->shm_perm.security;
+ isec = shp->security;
ad.type = LSM_AUDIT_DATA_IPC;
- ad.u.ipc_id = shp->shm_perm.key;
+ ad.u.ipc_id = shp->key;
return avc_has_perm(sid, isec->sid, SECCLASS_SHM,
SHM__ASSOCIATE, &ad);
}
/* Note, at this point, shp is locked down */
-static int selinux_shm_shmctl(struct shmid_kernel *shp, int cmd)
+static int selinux_shm_shmctl(struct kern_ipc_perm *shp, int cmd)
{
int perms;
int err;
@@ -5749,11 +5749,11 @@ static int selinux_shm_shmctl(struct shmid_kernel *shp, int cmd)
return 0;
}
- err = ipc_has_perm(&shp->shm_perm, perms);
+ err = ipc_has_perm(shp, perms);
return err;
}
-static int selinux_shm_shmat(struct shmid_kernel *shp,
+static int selinux_shm_shmat(struct kern_ipc_perm *shp,
char __user *shmaddr, int shmflg)
{
u32 perms;
@@ -5763,7 +5763,7 @@ static int selinux_shm_shmat(struct shmid_kernel *shp,
else
perms = SHM__READ | SHM__WRITE;
- return ipc_has_perm(&shp->shm_perm, perms);
+ return ipc_has_perm(shp, perms);
}
/* Semaphore security operations */
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 0402b8c1aec1..a3398c7f32c9 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2950,9 +2950,9 @@ static void smack_msg_msg_free_security(struct msg_msg *msg)
*
* Returns a pointer to the smack value
*/
-static struct smack_known *smack_of_shm(struct shmid_kernel *shp)
+static struct smack_known *smack_of_shm(struct kern_ipc_perm *shp)
{
- return (struct smack_known *)shp->shm_perm.security;
+ return (struct smack_known *)shp->security;
}
/**
@@ -2961,9 +2961,9 @@ static struct smack_known *smack_of_shm(struct shmid_kernel *shp)
*
* Returns 0
*/
-static int smack_shm_alloc_security(struct shmid_kernel *shp)
+static int smack_shm_alloc_security(struct kern_ipc_perm *shp)
{
- struct kern_ipc_perm *isp = &shp->shm_perm;
+ struct kern_ipc_perm *isp = shp;
struct smack_known *skp = smk_of_current();
isp->security = skp;
@@ -2976,9 +2976,9 @@ static int smack_shm_alloc_security(struct shmid_kernel *shp)
*
* Clears the blob pointer
*/
-static void smack_shm_free_security(struct shmid_kernel *shp)
+static void smack_shm_free_security(struct kern_ipc_perm *shp)
{
- struct kern_ipc_perm *isp = &shp->shm_perm;
+ struct kern_ipc_perm *isp = shp;
isp->security = NULL;
}
@@ -2990,7 +2990,7 @@ static void smack_shm_free_security(struct shmid_kernel *shp)
*
* Returns 0 if current has the requested access, error code otherwise
*/
-static int smk_curacc_shm(struct shmid_kernel *shp, int access)
+static int smk_curacc_shm(struct kern_ipc_perm *shp, int access)
{
struct smack_known *ssp = smack_of_shm(shp);
struct smk_audit_info ad;
@@ -2998,7 +2998,7 @@ static int smk_curacc_shm(struct shmid_kernel *shp, int access)
#ifdef CONFIG_AUDIT
smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_IPC);
- ad.a.u.ipc_id = shp->shm_perm.id;
+ ad.a.u.ipc_id = shp->id;
#endif
rc = smk_curacc(ssp, access, &ad);
rc = smk_bu_current("shm", ssp, access, rc);
@@ -3012,7 +3012,7 @@ static int smk_curacc_shm(struct shmid_kernel *shp, int access)
*
* Returns 0 if current has the requested access, error code otherwise
*/
-static int smack_shm_associate(struct shmid_kernel *shp, int shmflg)
+static int smack_shm_associate(struct kern_ipc_perm *shp, int shmflg)
{
int may;
@@ -3027,7 +3027,7 @@ static int smack_shm_associate(struct shmid_kernel *shp, int shmflg)
*
* Returns 0 if current has the requested access, error code otherwise
*/
-static int smack_shm_shmctl(struct shmid_kernel *shp, int cmd)
+static int smack_shm_shmctl(struct kern_ipc_perm *shp, int cmd)
{
int may;
@@ -3062,7 +3062,7 @@ static int smack_shm_shmctl(struct shmid_kernel *shp, int cmd)
*
* Returns 0 if current has the requested access, error code otherwise
*/
-static int smack_shm_shmat(struct shmid_kernel *shp, char __user *shmaddr,
+static int smack_shm_shmat(struct kern_ipc_perm *shp, char __user *shmaddr,
int shmflg)
{
int may;
--
2.14.1
^ permalink raw reply related
* [REVIEW][PATCH 01/11] sem/security: Pass kern_ipc_perm not sem_array into the sem security hooks
From: Eric W. Biederman @ 2018-03-23 19:16 UTC (permalink / raw)
To: Linux Containers
Cc: esyr-H+wXaHxf7aLQT0dZR+AlfA, jannh-hpIqsD4AKlfQT0dZR+AlfA,
khlebnikov-XoJtRXgx1JseBXzfvpsJ4g,
linux-api-u79uwXL29TY76Z2rM5mHXA,
serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
prakash.sangappa-QHcLZuEGTsvQT0dZR+AlfA,
linux-security-module-u79uwXL29TY76Z2rM5mHXA, Eric W. Biederman,
luto-DgEjT+Ai2ygdnm+yROfE0A, oleg-H+wXaHxf7aLQT0dZR+AlfA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b, Nagarathnam Muthusamy,
Pavel Emelyanov
In-Reply-To: <87vadmobdw.fsf_-_-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
All of the implementations of security hooks that take sem_array only
access sem_perm the struct kern_ipc_perm member. This means the
dependencies of the sem security hooks can be simplified by passing
the kern_ipc_perm member of sem_array.
Making this change will allow struct sem and struct sem_array
to become private to ipc/sem.c.
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
include/linux/lsm_hooks.h | 10 +++++-----
include/linux/security.h | 21 ++++++++++-----------
ipc/sem.c | 19 ++++++++-----------
security/security.c | 10 +++++-----
security/selinux/hooks.c | 28 ++++++++++++++--------------
security/smack/smack_lsm.c | 22 +++++++++++-----------
6 files changed, 53 insertions(+), 57 deletions(-)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 7161d8e7ee79..e4a94863a88c 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1592,11 +1592,11 @@ union security_list_options {
int (*shm_shmat)(struct shmid_kernel *shp, char __user *shmaddr,
int shmflg);
- int (*sem_alloc_security)(struct sem_array *sma);
- void (*sem_free_security)(struct sem_array *sma);
- int (*sem_associate)(struct sem_array *sma, int semflg);
- int (*sem_semctl)(struct sem_array *sma, int cmd);
- int (*sem_semop)(struct sem_array *sma, struct sembuf *sops,
+ int (*sem_alloc_security)(struct kern_ipc_perm *sma);
+ void (*sem_free_security)(struct kern_ipc_perm *sma);
+ int (*sem_associate)(struct kern_ipc_perm *sma, int semflg);
+ int (*sem_semctl)(struct kern_ipc_perm *sma, int cmd);
+ int (*sem_semop)(struct kern_ipc_perm *sma, struct sembuf *sops,
unsigned nsops, int alter);
int (*netlink_send)(struct sock *sk, struct sk_buff *skb);
diff --git a/include/linux/security.h b/include/linux/security.h
index 73f1ef625d40..fa7adac4b99a 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -36,7 +36,6 @@ struct linux_binprm;
struct cred;
struct rlimit;
struct siginfo;
-struct sem_array;
struct sembuf;
struct kern_ipc_perm;
struct audit_context;
@@ -368,11 +367,11 @@ void security_shm_free(struct shmid_kernel *shp);
int security_shm_associate(struct shmid_kernel *shp, int shmflg);
int security_shm_shmctl(struct shmid_kernel *shp, int cmd);
int security_shm_shmat(struct shmid_kernel *shp, char __user *shmaddr, int shmflg);
-int security_sem_alloc(struct sem_array *sma);
-void security_sem_free(struct sem_array *sma);
-int security_sem_associate(struct sem_array *sma, int semflg);
-int security_sem_semctl(struct sem_array *sma, int cmd);
-int security_sem_semop(struct sem_array *sma, struct sembuf *sops,
+int security_sem_alloc(struct kern_ipc_perm *sma);
+void security_sem_free(struct kern_ipc_perm *sma);
+int security_sem_associate(struct kern_ipc_perm *sma, int semflg);
+int security_sem_semctl(struct kern_ipc_perm *sma, int cmd);
+int security_sem_semop(struct kern_ipc_perm *sma, struct sembuf *sops,
unsigned nsops, int alter);
void security_d_instantiate(struct dentry *dentry, struct inode *inode);
int security_getprocattr(struct task_struct *p, char *name, char **value);
@@ -1103,25 +1102,25 @@ static inline int security_shm_shmat(struct shmid_kernel *shp,
return 0;
}
-static inline int security_sem_alloc(struct sem_array *sma)
+static inline int security_sem_alloc(struct kern_ipc_perm *sma)
{
return 0;
}
-static inline void security_sem_free(struct sem_array *sma)
+static inline void security_sem_free(struct kern_ipc_perm *sma)
{ }
-static inline int security_sem_associate(struct sem_array *sma, int semflg)
+static inline int security_sem_associate(struct kern_ipc_perm *sma, int semflg)
{
return 0;
}
-static inline int security_sem_semctl(struct sem_array *sma, int cmd)
+static inline int security_sem_semctl(struct kern_ipc_perm *sma, int cmd)
{
return 0;
}
-static inline int security_sem_semop(struct sem_array *sma,
+static inline int security_sem_semop(struct kern_ipc_perm *sma,
struct sembuf *sops, unsigned nsops,
int alter)
{
diff --git a/ipc/sem.c b/ipc/sem.c
index a4af04979fd2..01f5c63670ae 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -265,7 +265,7 @@ static void sem_rcu_free(struct rcu_head *head)
struct kern_ipc_perm *p = container_of(head, struct kern_ipc_perm, rcu);
struct sem_array *sma = container_of(p, struct sem_array, sem_perm);
- security_sem_free(sma);
+ security_sem_free(&sma->sem_perm);
kvfree(sma);
}
@@ -495,7 +495,7 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params)
sma->sem_perm.key = key;
sma->sem_perm.security = NULL;
- retval = security_sem_alloc(sma);
+ retval = security_sem_alloc(&sma->sem_perm);
if (retval) {
kvfree(sma);
return retval;
@@ -535,10 +535,7 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params)
*/
static inline int sem_security(struct kern_ipc_perm *ipcp, int semflg)
{
- struct sem_array *sma;
-
- sma = container_of(ipcp, struct sem_array, sem_perm);
- return security_sem_associate(sma, semflg);
+ return security_sem_associate(ipcp, semflg);
}
/*
@@ -1209,7 +1206,7 @@ static int semctl_stat(struct ipc_namespace *ns, int semid,
if (ipcperms(ns, &sma->sem_perm, S_IRUGO))
goto out_unlock;
- err = security_sem_semctl(sma, cmd);
+ err = security_sem_semctl(&sma->sem_perm, cmd);
if (err)
goto out_unlock;
@@ -1300,7 +1297,7 @@ static int semctl_setval(struct ipc_namespace *ns, int semid, int semnum,
return -EACCES;
}
- err = security_sem_semctl(sma, SETVAL);
+ err = security_sem_semctl(&sma->sem_perm, SETVAL);
if (err) {
rcu_read_unlock();
return -EACCES;
@@ -1354,7 +1351,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
if (ipcperms(ns, &sma->sem_perm, cmd == SETALL ? S_IWUGO : S_IRUGO))
goto out_rcu_wakeup;
- err = security_sem_semctl(sma, cmd);
+ err = security_sem_semctl(&sma->sem_perm, cmd);
if (err)
goto out_rcu_wakeup;
@@ -1545,7 +1542,7 @@ static int semctl_down(struct ipc_namespace *ns, int semid,
sma = container_of(ipcp, struct sem_array, sem_perm);
- err = security_sem_semctl(sma, cmd);
+ err = security_sem_semctl(&sma->sem_perm, cmd);
if (err)
goto out_unlock1;
@@ -1962,7 +1959,7 @@ static long do_semtimedop(int semid, struct sembuf __user *tsops,
goto out_free;
}
- error = security_sem_semop(sma, sops, nsops, alter);
+ error = security_sem_semop(&sma->sem_perm, sops, nsops, alter);
if (error) {
rcu_read_unlock();
goto out_free;
diff --git a/security/security.c b/security/security.c
index 1cd8526cb0b7..d3b9aeb6b73b 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1220,27 +1220,27 @@ int security_shm_shmat(struct shmid_kernel *shp, char __user *shmaddr, int shmfl
return call_int_hook(shm_shmat, 0, shp, shmaddr, shmflg);
}
-int security_sem_alloc(struct sem_array *sma)
+int security_sem_alloc(struct kern_ipc_perm *sma)
{
return call_int_hook(sem_alloc_security, 0, sma);
}
-void security_sem_free(struct sem_array *sma)
+void security_sem_free(struct kern_ipc_perm *sma)
{
call_void_hook(sem_free_security, sma);
}
-int security_sem_associate(struct sem_array *sma, int semflg)
+int security_sem_associate(struct kern_ipc_perm *sma, int semflg)
{
return call_int_hook(sem_associate, 0, sma, semflg);
}
-int security_sem_semctl(struct sem_array *sma, int cmd)
+int security_sem_semctl(struct kern_ipc_perm *sma, int cmd)
{
return call_int_hook(sem_semctl, 0, sma, cmd);
}
-int security_sem_semop(struct sem_array *sma, struct sembuf *sops,
+int security_sem_semop(struct kern_ipc_perm *sma, struct sembuf *sops,
unsigned nsops, int alter)
{
return call_int_hook(sem_semop, 0, sma, sops, nsops, alter);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 8644d864e3c1..cce994e9fc0a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5767,53 +5767,53 @@ static int selinux_shm_shmat(struct shmid_kernel *shp,
}
/* Semaphore security operations */
-static int selinux_sem_alloc_security(struct sem_array *sma)
+static int selinux_sem_alloc_security(struct kern_ipc_perm *sma)
{
struct ipc_security_struct *isec;
struct common_audit_data ad;
u32 sid = current_sid();
int rc;
- rc = ipc_alloc_security(&sma->sem_perm, SECCLASS_SEM);
+ rc = ipc_alloc_security(sma, SECCLASS_SEM);
if (rc)
return rc;
- isec = sma->sem_perm.security;
+ isec = sma->security;
ad.type = LSM_AUDIT_DATA_IPC;
- ad.u.ipc_id = sma->sem_perm.key;
+ ad.u.ipc_id = sma->key;
rc = avc_has_perm(sid, isec->sid, SECCLASS_SEM,
SEM__CREATE, &ad);
if (rc) {
- ipc_free_security(&sma->sem_perm);
+ ipc_free_security(sma);
return rc;
}
return 0;
}
-static void selinux_sem_free_security(struct sem_array *sma)
+static void selinux_sem_free_security(struct kern_ipc_perm *sma)
{
- ipc_free_security(&sma->sem_perm);
+ ipc_free_security(sma);
}
-static int selinux_sem_associate(struct sem_array *sma, int semflg)
+static int selinux_sem_associate(struct kern_ipc_perm *sma, int semflg)
{
struct ipc_security_struct *isec;
struct common_audit_data ad;
u32 sid = current_sid();
- isec = sma->sem_perm.security;
+ isec = sma->security;
ad.type = LSM_AUDIT_DATA_IPC;
- ad.u.ipc_id = sma->sem_perm.key;
+ ad.u.ipc_id = sma->key;
return avc_has_perm(sid, isec->sid, SECCLASS_SEM,
SEM__ASSOCIATE, &ad);
}
/* Note, at this point, sma is locked down */
-static int selinux_sem_semctl(struct sem_array *sma, int cmd)
+static int selinux_sem_semctl(struct kern_ipc_perm *sma, int cmd)
{
int err;
u32 perms;
@@ -5851,11 +5851,11 @@ static int selinux_sem_semctl(struct sem_array *sma, int cmd)
return 0;
}
- err = ipc_has_perm(&sma->sem_perm, perms);
+ err = ipc_has_perm(sma, perms);
return err;
}
-static int selinux_sem_semop(struct sem_array *sma,
+static int selinux_sem_semop(struct kern_ipc_perm *sma,
struct sembuf *sops, unsigned nsops, int alter)
{
u32 perms;
@@ -5865,7 +5865,7 @@ static int selinux_sem_semop(struct sem_array *sma,
else
perms = SEM__READ;
- return ipc_has_perm(&sma->sem_perm, perms);
+ return ipc_has_perm(sma, perms);
}
static int selinux_ipc_permission(struct kern_ipc_perm *ipcp, short flag)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 03fdecba93bb..0402b8c1aec1 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3077,9 +3077,9 @@ static int smack_shm_shmat(struct shmid_kernel *shp, char __user *shmaddr,
*
* Returns a pointer to the smack value
*/
-static struct smack_known *smack_of_sem(struct sem_array *sma)
+static struct smack_known *smack_of_sem(struct kern_ipc_perm *sma)
{
- return (struct smack_known *)sma->sem_perm.security;
+ return (struct smack_known *)sma->security;
}
/**
@@ -3088,9 +3088,9 @@ static struct smack_known *smack_of_sem(struct sem_array *sma)
*
* Returns 0
*/
-static int smack_sem_alloc_security(struct sem_array *sma)
+static int smack_sem_alloc_security(struct kern_ipc_perm *sma)
{
- struct kern_ipc_perm *isp = &sma->sem_perm;
+ struct kern_ipc_perm *isp = sma;
struct smack_known *skp = smk_of_current();
isp->security = skp;
@@ -3103,9 +3103,9 @@ static int smack_sem_alloc_security(struct sem_array *sma)
*
* Clears the blob pointer
*/
-static void smack_sem_free_security(struct sem_array *sma)
+static void smack_sem_free_security(struct kern_ipc_perm *sma)
{
- struct kern_ipc_perm *isp = &sma->sem_perm;
+ struct kern_ipc_perm *isp = sma;
isp->security = NULL;
}
@@ -3117,7 +3117,7 @@ static void smack_sem_free_security(struct sem_array *sma)
*
* Returns 0 if current has the requested access, error code otherwise
*/
-static int smk_curacc_sem(struct sem_array *sma, int access)
+static int smk_curacc_sem(struct kern_ipc_perm *sma, int access)
{
struct smack_known *ssp = smack_of_sem(sma);
struct smk_audit_info ad;
@@ -3125,7 +3125,7 @@ static int smk_curacc_sem(struct sem_array *sma, int access)
#ifdef CONFIG_AUDIT
smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_IPC);
- ad.a.u.ipc_id = sma->sem_perm.id;
+ ad.a.u.ipc_id = sma->id;
#endif
rc = smk_curacc(ssp, access, &ad);
rc = smk_bu_current("sem", ssp, access, rc);
@@ -3139,7 +3139,7 @@ static int smk_curacc_sem(struct sem_array *sma, int access)
*
* Returns 0 if current has the requested access, error code otherwise
*/
-static int smack_sem_associate(struct sem_array *sma, int semflg)
+static int smack_sem_associate(struct kern_ipc_perm *sma, int semflg)
{
int may;
@@ -3154,7 +3154,7 @@ static int smack_sem_associate(struct sem_array *sma, int semflg)
*
* Returns 0 if current has the requested access, error code otherwise
*/
-static int smack_sem_semctl(struct sem_array *sma, int cmd)
+static int smack_sem_semctl(struct kern_ipc_perm *sma, int cmd)
{
int may;
@@ -3198,7 +3198,7 @@ static int smack_sem_semctl(struct sem_array *sma, int cmd)
*
* Returns 0 if access is allowed, error code otherwise
*/
-static int smack_sem_semop(struct sem_array *sma, struct sembuf *sops,
+static int smack_sem_semop(struct kern_ipc_perm *sma, struct sembuf *sops,
unsigned nsops, int alter)
{
return smk_curacc_sem(sma, MAY_READWRITE);
--
2.14.1
^ permalink raw reply related
* [REVIEW][PATCH 00/11] ipc: Fixing the pid namespace support
From: Eric W. Biederman @ 2018-03-23 19:11 UTC (permalink / raw)
To: Linux Containers
Cc: esyr-H+wXaHxf7aLQT0dZR+AlfA, jannh-hpIqsD4AKlfQT0dZR+AlfA,
khlebnikov-XoJtRXgx1JseBXzfvpsJ4g,
linux-api-u79uwXL29TY76Z2rM5mHXA,
serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
prakash.sangappa-QHcLZuEGTsvQT0dZR+AlfA,
linux-security-module-u79uwXL29TY76Z2rM5mHXA,
luto-DgEjT+Ai2ygdnm+yROfE0A, oleg-H+wXaHxf7aLQT0dZR+AlfA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b, Nagarathnam Muthusamy,
Pavel Emelyanov
In-Reply-To: <87a7v2z2qa.fsf-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
While discussing a proposal by Nagarathnam Muthusamy I realized that
the ipc namespace has never been given proper pid namespace support.
In particular if objects in a single ipc namespace are accessed from
different pid namespaces they will return the wrong pids.
Then when trying to test this I discovered that definitions that are
only used in msg.c, sem.c, and shm.c are included in linux/sched.h
resulting in what should be simple local modifications requring
nearly a full kernel rebuild.
So this patchset does several things.
- Changes the types that are passed into the security hooks to struct
kern_ipc_perm because that is all the security hooks use.
- Moves definitions from include/{msg,sem,shm}.h into ipc/{msg,sem,shm}.c
So the code can be modified without excessive development time.
- Instead of storing pids as intergers stores struct pid * instead.
I took a careful look to see if it seems likely the performance
regression in credential passing that af_unix experienced after
a similar conversion would be likely, but I don't see it.
So I think the biggest concern is if someone in the last 10 years
has come to depend on the buggy behavior. If either the performance
is problematic or the there are regression caused by the change
in behavior we can revert.
Still I would like to see this fixed and I plan on merging this code.
Eric W. Biederman (11):
sem/security: Pass kern_ipc_perm not sem_array into the sem security hooks
shm/security: Pass kern_ipc_perm not shmid_kernel into the shm security hooks
msg/security: Pass kern_ipc_perm not msg_queue into the msg_queue security hooks
sem: Move struct sem and struct sem_array into ipc/sem.c
shm: Move struct shmid_kernel into ipc/shm.c
msg: Move struct msg_queue into ipc/msg.c
ipc: Move IPCMNI from include/ipc.h into ipc/util.h
ipc/util: Helpers for making the sysvipc operations pid namespace aware
ipc/shm: Fix shmctl(..., IPC_STAT, ...) between pid namespaces.
ipc/msg: Fix msgctl(..., IPC_STAT, ...) between pid namespaces
ipc/sem: Fix semctl(..., GETPID, ...) between pid namespaces
include/linux/ipc.h | 2 -
include/linux/lsm_hooks.h | 32 ++++++++--------
include/linux/msg.h | 18 ---------
include/linux/security.h | 67 ++++++++++++++++-----------------
include/linux/sem.h | 40 +-------------------
include/linux/shm.h | 23 ------------
ipc/msg.c | 54 ++++++++++++++++++---------
ipc/sem.c | 73 ++++++++++++++++++++++++++----------
ipc/shm.c | 60 +++++++++++++++++++++---------
ipc/util.c | 9 +++++
ipc/util.h | 12 ++++++
security/security.c | 32 ++++++++--------
security/selinux/hooks.c | 92 +++++++++++++++++++++++-----------------------
security/smack/smack_lsm.c | 68 +++++++++++++++++-----------------
14 files changed, 297 insertions(+), 285 deletions(-)
Eric
^ permalink raw reply
* Re: [PATCH 06/28] aio: implement IOCB_CMD_POLL
From: Christoph Hellwig @ 2018-03-23 18:05 UTC (permalink / raw)
To: Al Viro
Cc: Christoph Hellwig, Avi Kivity, linux-aio, linux-fsdevel, netdev,
linux-api, linux-kernel
In-Reply-To: <20180322181653.GJ30522@ZenIV.linux.org.uk>
On Thu, Mar 22, 2018 at 06:16:53PM +0000, Al Viro wrote:
> On Thu, Mar 22, 2018 at 06:24:10PM +0100, Christoph Hellwig wrote:
>
> > -static void aio_complete(struct aio_kiocb *iocb, long res, long res2)
> > +static bool aio_complete(struct aio_kiocb *iocb, long res, long res2,
> > + unsigned complete_flags)
>
> Looks like all callers are following that with "if returned true,
> fput(something)". Does it really make any sense to keep that struct
> file * in different fields?
struct kiocb is used not just for aio, but for our normal read/write_iter
APIs, and it is not suitable for poll or fsync. So I can't really find
a good way to keep it common except for duplicating it in struct kiocb
and strut aio_iocb. But maybe we could pass a struct file argument
to aio_complete().
> Wait a sec... What ordering do we want for
> * call(s) of ->ki_complete
> * call (if any) of ->ki_cancel
> * dropping reference to struct file
> and what are the expected call chains for all of those?
fput must be done exactly once from inside ->ki_complete OR ->ki_cancel
in case it did manage to do the actual completion. Reference to struct
file isn't needed in aio_complete, but if aio_complete decided who
won the race we'll have to put after it (or inside it if we want to make
it common)
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH v4 04/24] fpga: add device feature list support
From: Wu Hao @ 2018-03-23 4:33 UTC (permalink / raw)
To: Alan Tull
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <CANk1AXSaE3wQ7jnBkB0TR3kLKnG1XUeHk+LbDSxyRtBxbMNGPw@mail.gmail.com>
On Thu, Mar 22, 2018 at 04:31:05PM -0500, Alan Tull wrote:
> On Tue, Feb 13, 2018 at 3:24 AM, Wu Hao <hao.wu@intel.com> wrote:
>
> Hi Hao,
Hi Alan
Thanks a lot for the code review and the comments. : )
>
> > Device Feature List (DFL) defines a feature list structure that creates
> > a link list of feature headers within the MMIO space to provide an
> > extensible way of adding features. This patch introduces a kernel module
> > to provide basic infrastructure to support FPGA devices which implement
> > the Device Feature List.
> >
> > Usually there will be different features and their sub features linked into
> > the DFL. This code provides common APIs for feature enumeration, it creates
> > a container device (FPGA base region), walks through the DFLs and creates
> > platform devices for feature devices (Currently it only supports two
> > different feature devices, FPGA Management Engine (FME) and Port which
> > the Accelerator Function Unit (AFU) connected to). In order to enumerate
> > the DFLs, the common APIs required low level driver to provide necessary
> > enumeration information (e.g address for each device feature list for
> > given device) and fill it to the fpga_enum_info data structure. Please
> > refer to below description for APIs added for enumeration.
> >
> > Functions for enumeration information preparation:
> > *fpga_enum_info_alloc
> > allocate enumeration information data structure.
> >
> > *fpga_enum_info_add_dfl
> > add a device feature list to fpga_enum_info data structure.
> >
> > *fpga_enum_info_free
> > free fpga_enum_info data structure and related resources.
> >
> > Functions for feature device enumeration:
> > *fpga_enumerate_feature_devs
> > enumerate feature devices and return container device.
> >
> > *fpga_remove_feature_devs
> > remove feature devices under given container device.
>
> This header doesn't say anything about the reset or bridge
> functionality (fpga_port_enable/disable/reset) that's added here and
> used elsewhere in the patch set.
Sure, I will add descriptions for these functions in the next version.
>
> >
> > Signed-off-by: Tim Whisonant <tim.whisonant@intel.com>
> > Signed-off-by: Enno Luebbers <enno.luebbers@intel.com>
> > Signed-off-by: Shiva Rao <shiva.rao@intel.com>
> > Signed-off-by: Christopher Rauer <christopher.rauer@intel.com>
> > Signed-off-by: Zhang Yi <yi.z.zhang@intel.com>
> > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > ---
> > v3: split from another patch.
> > separate dfl enumeration code from original pcie driver.
> > provide common data structures and APIs for enumeration.
> > update device feature list parsing process according to latest hw.
> > add dperf/iperf/hssi sub feature placeholder according to latest hw.
> > remove build_info_add_sub_feature and other small functions.
> > replace *_feature_num function with macro.
> > remove writeq/readq.
> > v4: fix SPDX license issue
> > rename files to dfl.[ch], fix typo and add more comments.
> > remove static feature_info tables for FME and Port.
> > remove check on next_afu link list as only FIU has next_afu ptr.
> > remove unused macro in header file.
> > add more comments for functions.
> > ---
> > drivers/fpga/Kconfig | 16 +
> > drivers/fpga/Makefile | 3 +
> > drivers/fpga/dfl.c | 787 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > drivers/fpga/dfl.h | 345 ++++++++++++++++++++++
> > 4 files changed, 1151 insertions(+)
> > create mode 100644 drivers/fpga/dfl.c
> > create mode 100644 drivers/fpga/dfl.h
> >
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > index f47ef84..01ad31f 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -124,4 +124,20 @@ config OF_FPGA_REGION
> > Support for loading FPGA images by applying a Device Tree
> > overlay.
> >
> > +config FPGA_DFL
> > + tristate "FPGA Device Feature List (DFL) support"
> > + select FPGA_BRIDGE
> > + select FPGA_REGION
> > + help
> > + Device Feature List (DFL) defines a feature list structure that
> > + creates a link list of feature headers within the MMIO space
> > + to provide an extensible way of adding features for FPGA.
> > + Driver can walk through the feature headers to enumerate feature
> > + devices (e.g FPGA Management Engine, Port and Accelerator
> > + Function Unit) and their private features for target FPGA devices.
> > +
> > + Select this option to enable common support for Field-Programmable
> > + Gate Array (FPGA) solutions which implement Device Feature List.
> > + It provides enumeration APIs, and feature device infrastructure.
> > +
> > endif # FPGA
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > index 3cb276a..c4c62b9 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -27,3 +27,6 @@ obj-$(CONFIG_XILINX_PR_DECOUPLER) += xilinx-pr-decoupler.o
> > # High Level Interfaces
> > obj-$(CONFIG_FPGA_REGION) += fpga-region.o
> > obj-$(CONFIG_OF_FPGA_REGION) += of-fpga-region.o
> > +
> > +# FPGA Device Feature List Support
> > +obj-$(CONFIG_FPGA_DFL) += dfl.o
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > new file mode 100644
> > index 0000000..f50694e
> > --- /dev/null
> > +++ b/drivers/fpga/dfl.c
> > @@ -0,0 +1,787 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for FPGA Device Feature List (DFL) Support
> > + *
> > + * Copyright (C) 2017 Intel Corporation, Inc.
> > + *
> > + * Authors:
> > + * Kang Luwei <luwei.kang@intel.com>
> > + * Zhang Yi <yi.z.zhang@intel.com>
> > + * Wu Hao <hao.wu@intel.com>
> > + * Xiao Guangrong <guangrong.xiao@linux.intel.com>
> > + */
> > +#include <linux/module.h>
> > +
> > +#include "dfl.h"
> > +
> > +static DEFINE_MUTEX(fpga_id_mutex);
> > +
> > +enum fpga_id_type {
> > + FME_ID, /* fme id allocation and mapping */
> > + PORT_ID, /* port id allocation and mapping */
> > + FPGA_ID_MAX,
> > +};
> > +
> > +/* it is protected by fpga_id_mutex */
> > +static struct idr fpga_ids[FPGA_ID_MAX];
> > +
> > +static void fpga_ids_init(void)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(fpga_ids); i++)
> > + idr_init(fpga_ids + i);
> > +}
> > +
> > +static void fpga_ids_destroy(void)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(fpga_ids); i++)
> > + idr_destroy(fpga_ids + i);
> > +}
> > +
> > +static int alloc_fpga_id(enum fpga_id_type type, struct device *dev)
> > +{
> > + int id;
> > +
> > + WARN_ON(type >= FPGA_ID_MAX);
> > + mutex_lock(&fpga_id_mutex);
> > + id = idr_alloc(fpga_ids + type, dev, 0, 0, GFP_KERNEL);
> > + mutex_unlock(&fpga_id_mutex);
> > +
> > + return id;
> > +}
> > +
> > +static void free_fpga_id(enum fpga_id_type type, int id)
> > +{
> > + WARN_ON(type >= FPGA_ID_MAX);
> > + mutex_lock(&fpga_id_mutex);
> > + idr_remove(fpga_ids + type, id);
> > + mutex_unlock(&fpga_id_mutex);
> > +}
> > +
> > +static enum fpga_id_type feature_dev_id_type(struct platform_device *pdev)
> > +{
> > + if (!strcmp(pdev->name, FPGA_FEATURE_DEV_FME))
> > + return FME_ID;
> > +
> > + if (!strcmp(pdev->name, FPGA_FEATURE_DEV_PORT))
> > + return PORT_ID;
> > +
> > + WARN_ON(1);
> > +
> > + return FPGA_ID_MAX;
> > +}
> > +
> > +/**
> > + * struct build_feature_devs_info - info collected during feature dev build.
> > + *
> > + * @dev: device to enumerate.
> > + * @cdev: the container device for all feature devices.
> > + * @feature_dev: current feature device.
> > + * @ioaddr: header register region address of feature device in enumeration.
> > + * @sub_features: a sub features link list for feature device in enumeration.
> > + * @feature_num: number of sub features for feature device in enumeration.
> > + */
> > +struct build_feature_devs_info {
> > + struct device *dev;
> > + struct fpga_cdev *cdev;
> > + struct platform_device *feature_dev;
> > + void __iomem *ioaddr;
> > + struct list_head sub_features;
> > + int feature_num;
> > +};
> > +
> > +/**
> > + * struct feature_info - sub feature info collected during feature dev build.
> > + *
> > + * @fid: id of this sub feature.
> > + * @mmio_res: mmio resource of this sub feature.
> > + * @ioaddr: mapped base address of mmio resource.
> > + * @node: node in sub_features link list.
> > + */
> > +struct feature_info {
> > + u64 fid;
> > + struct resource mmio_res;
> > + void __iomem *ioaddr;
> > + struct list_head node;
> > +};
> > +
> > +static void fpga_cdev_add_port_dev(struct fpga_cdev *cdev,
> > + struct platform_device *port_pdev)
> > +{
> > + struct feature_platform_data *pdata = dev_get_platdata(&port_pdev->dev);
> > +
> > + mutex_lock(&cdev->lock);
> > + list_add(&pdata->node, &cdev->port_dev_list);
> > + get_device(&pdata->dev->dev);
> > + mutex_unlock(&cdev->lock);
> > +}
> > +
> > +/*
> > + * register current feature device, it is called when we need to switch to
> > + * another feature parsing or we have parsed all features on given device
> > + * feature list.
> > + */
> > +static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> > +{
> > + struct platform_device *fdev = binfo->feature_dev;
> > + struct feature_platform_data *pdata;
> > + struct feature_info *finfo, *p;
> > + int ret, index = 0;
> > +
> > + if (!fdev)
> > + return 0;
> > +
> > + /*
> > + * we do not need to care for the memory which is associated with
> > + * the platform device. After calling platform_device_unregister(),
> > + * it will be automatically freed by device's release() callback,
> > + * platform_device_release().
> > + */
> > + pdata = kzalloc(feature_platform_data_size(binfo->feature_num),
> > + GFP_KERNEL);
> > + if (pdata) {
> > + pdata->dev = fdev;
> > + pdata->num = binfo->feature_num;
> > + mutex_init(&pdata->lock);
> > + } else {
> > + return -ENOMEM;
> > + }
> > +
> > + /*
> > + * the count should be initialized to 0 to make sure
> > + *__fpga_port_enable() following __fpga_port_disable()
> > + * works properly for port device.
> > + * and it should always be 0 for fme device.
> > + */
> > + WARN_ON(pdata->disable_count);
> > +
> > + fdev->dev.platform_data = pdata;
> > +
> > + /* each sub feature has one MMIO resource */
> > + fdev->num_resources = binfo->feature_num;
> > + fdev->resource = kcalloc(binfo->feature_num, sizeof(*fdev->resource),
> > + GFP_KERNEL);
> > + if (!fdev->resource)
> > + return -ENOMEM;
> > +
> > + /* fill features and resource information for feature dev */
> > + list_for_each_entry_safe(finfo, p, &binfo->sub_features, node) {
> > + struct feature *feature = &pdata->features[index];
> > +
> > + /* save resource information for each feature */
> > + feature->id = finfo->fid;
> > + feature->resource_index = index;
> > + feature->ioaddr = finfo->ioaddr;
> > + fdev->resource[index++] = finfo->mmio_res;
> > +
> > + list_del(&finfo->node);
> > + kfree(finfo);
> > + }
> > +
> > + ret = platform_device_add(binfo->feature_dev);
> > + if (!ret) {
> > + if (feature_dev_id_type(binfo->feature_dev) == PORT_ID)
> > + fpga_cdev_add_port_dev(binfo->cdev, binfo->feature_dev);
> > + else
> > + binfo->cdev->fme_dev =
> > + get_device(&binfo->feature_dev->dev);
> > + /*
> > + * reset it to avoid build_info_free() freeing their resource.
> > + *
> > + * The resource of successfully registered feature devices
> > + * will be freed by platform_device_unregister(). See the
> > + * comments in build_info_create_dev().
> > + */
> > + binfo->feature_dev = NULL;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int
> > +build_info_create_dev(struct build_feature_devs_info *binfo,
> > + enum fpga_id_type type, const char *name,
> > + void __iomem *ioaddr)
> > +{
> > + struct platform_device *fdev;
> > + int ret;
> > +
> > + /* we will create a new device, commit current device first */
> > + ret = build_info_commit_dev(binfo);
> > + if (ret)
> > + return ret;
> > +
> > + /*
> > + * we use -ENODEV as the initialization indicator which indicates
> > + * whether the id need to be reclaimed
> > + */
> > + fdev = platform_device_alloc(name, -ENODEV);
> > + if (!fdev)
> > + return -ENOMEM;
> > +
> > + binfo->feature_dev = fdev;
> > + binfo->feature_num = 0;
> > + binfo->ioaddr = ioaddr;
> > + INIT_LIST_HEAD(&binfo->sub_features);
> > +
> > + fdev->id = alloc_fpga_id(type, &fdev->dev);
> > + if (fdev->id < 0)
> > + return fdev->id;
> > +
> > + fdev->dev.parent = &binfo->cdev->region.dev;
> > +
> > + return 0;
> > +}
> > +
> > +static void build_info_free(struct build_feature_devs_info *binfo)
> > +{
> > + struct feature_info *finfo, *p;
> > +
> > + /*
> > + * it is a valid id, free it. See comments in
> > + * build_info_create_dev()
> > + */
> > + if (binfo->feature_dev && binfo->feature_dev->id >= 0) {
> > + free_fpga_id(feature_dev_id_type(binfo->feature_dev),
> > + binfo->feature_dev->id);
> > +
> > + list_for_each_entry_safe(finfo, p, &binfo->sub_features, node) {
> > + list_del(&finfo->node);
> > + kfree(finfo);
> > + }
> > + }
> > +
> > + platform_device_put(binfo->feature_dev);
> > +
> > + devm_kfree(binfo->dev, binfo);
> > +}
> > +
> > +static inline u32 feature_size(void __iomem *start)
> > +{
> > + u64 v = readq(start + DFH);
> > + u32 ofst = FIELD_GET(DFH_NEXT_HDR_OFST, v);
> > + /* workaround for private features with invalid size, use 4K instead */
> > + return ofst ? ofst : 4096;
> > +}
> > +
> > +static u64 feature_id(void __iomem *start)
> > +{
> > + u64 v = readq(start + DFH);
> > + u16 id = FIELD_GET(DFH_ID, v);
> > + u8 type = FIELD_GET(DFH_TYPE, v);
> > +
> > + if (type == DFH_TYPE_FIU)
> > + return FEATURE_ID_FIU_HEADER;
> > + else if (type == DFH_TYPE_PRIVATE)
> > + return id;
> > + else if (type == DFH_TYPE_AFU)
> > + return FEATURE_ID_AFU;
> > +
> > + WARN_ON(1);
> > + return 0;
> > +}
> > +
> > +/*
> > + * when create sub feature instances, for private features, it doesn't need
> > + * to provide resource size and feature id as they could be read from DFH
> > + * register. For afu sub feature, its register region only contains user
> > + * defined registers, so never trust any information from it, just use the
> > + * resource size information provided by its parent FIU.
> > + */
> > +static int
> > +create_feature_instance(struct build_feature_devs_info *binfo,
> > + struct fpga_enum_dfl *dfl, resource_size_t ofst,
> > + resource_size_t size, u64 fid)
> > +{
> > + struct feature_info *finfo;
> > +
> > + /* read feature size and id if inputs are invalid */
> > + size = size ? size : feature_size(dfl->ioaddr + ofst);
> > + fid = fid ? fid : feature_id(dfl->ioaddr + ofst);
> > +
> > + if (dfl->len - ofst < size)
> > + return -EINVAL;
> > +
> > + finfo = kzalloc(sizeof(*finfo), GFP_KERNEL);
> > + if (!finfo)
> > + return -ENOMEM;
> > +
> > + finfo->fid = fid;
> > + finfo->mmio_res.start = dfl->start + ofst;
> > + finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> > + finfo->mmio_res.flags = IORESOURCE_MEM;
> > + finfo->ioaddr = dfl->ioaddr + ofst;
> > +
> > + list_add_tail(&finfo->node, &binfo->sub_features);
> > + binfo->feature_num++;
> > +
> > + return 0;
> > +}
> > +
> > +static int parse_feature_fme(struct build_feature_devs_info *binfo,
> > + struct fpga_enum_dfl *dfl, resource_size_t ofst)
> > +{
> > + int ret;
> > +
> > + ret = build_info_create_dev(binfo, FME_ID, FPGA_FEATURE_DEV_FME,
> > + dfl->ioaddr + ofst);
> > + if (ret)
> > + return ret;
> > +
> > + return create_feature_instance(binfo, dfl, ofst, 0, 0);
> > +}
> > +
> > +static int parse_feature_port(struct build_feature_devs_info *binfo,
> > + struct fpga_enum_dfl *dfl,
> > + resource_size_t ofst)
> > +{
> > + int ret;
> > +
> > + ret = build_info_create_dev(binfo, PORT_ID, FPGA_FEATURE_DEV_PORT,
> > + dfl->ioaddr + ofst);
> > + if (ret)
> > + return ret;
> > +
> > + return create_feature_instance(binfo, dfl, ofst, 0, 0);
> > +}
> > +
> > +static int parse_feature_port_afu(struct build_feature_devs_info *binfo,
> > + struct fpga_enum_dfl *dfl,
> > + resource_size_t ofst)
> > +{
> > + u64 v = readq(binfo->ioaddr + PORT_HDR_CAP);
> > + u32 size = FIELD_GET(PORT_CAP_MMIO_SIZE, v) << 10;
> > +
> > + WARN_ON(!size);
> > +
> > + return create_feature_instance(binfo, dfl, ofst, size, FEATURE_ID_AFU);
> > +}
> > +
> > +static int parse_feature_afu(struct build_feature_devs_info *binfo,
> > + struct fpga_enum_dfl *dfl,
> > + resource_size_t ofst)
> > +{
> > + if (!binfo->feature_dev) {
> > + dev_err(binfo->dev, "this AFU does not belong to any FIU.\n");
> > + return -EINVAL;
> > + }
> > +
> > + switch (feature_dev_id_type(binfo->feature_dev)) {
> > + case PORT_ID:
> > + return parse_feature_port_afu(binfo, dfl, ofst);
> > + default:
> > + dev_info(binfo->dev, "AFU belonging to FIU %s is not supported yet.\n",
> > + binfo->feature_dev->name);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int parse_feature_fiu(struct build_feature_devs_info *binfo,
> > + struct fpga_enum_dfl *dfl,
> > + resource_size_t ofst)
> > +{
> > + u32 id, offset;
> > + u64 v;
> > + int ret = 0;
> > +
> > + v = readq(dfl->ioaddr + ofst + DFH);
> > + id = FIELD_GET(DFH_ID, v);
> > +
> > + switch (id) {
> > + case DFH_ID_FIU_FME:
> > + ret = parse_feature_fme(binfo, dfl, ofst);
> > + break;
> > + case DFH_ID_FIU_PORT:
> > + ret = parse_feature_port(binfo, dfl, ofst);
> > + break;
> > + default:
> > + dev_info(binfo->dev, "FIU TYPE %d is not supported yet.\n",
> > + id);
> > + }
> > +
> > + if (ret)
> > + return ret;
> > +
> > + /* Find and parse FIU's child AFU via its NEXT_AFU register */
> > + v = readq(dfl->ioaddr + ofst + NEXT_AFU);
> > +
> > + offset = FIELD_GET(NEXT_AFU_NEXT_DFH_OFST, v);
> > + if (offset)
> > + return parse_feature_afu(binfo, dfl, ofst + offset);
> > +
> > + dev_dbg(binfo->dev, "No AFUs detected on FIU %d\n", id);
> > +
> > + return ret;
> > +}
> > +
> > +static int parse_feature_private(struct build_feature_devs_info *binfo,
> > + struct fpga_enum_dfl *dfl,
> > + resource_size_t ofst)
> > +{
> > + if (!binfo->feature_dev) {
> > + dev_err(binfo->dev, "the private feature %llx does not belong to any AFU.\n",
> > + (unsigned long long)feature_id(dfl->ioaddr + ofst));
> > + return -EINVAL;
> > + }
> > +
> > + return create_feature_instance(binfo, dfl, ofst, 0, 0);
> > +}
> > +
> > +/**
> > + * parse_feature - parse a feature on given device feature list
> > + *
> > + * @binfo: build feature devices information.
> > + * @dfl: device feature list to parse
> > + * @ofst: offset to feature header on this device feature list
> > + */
> > +static int parse_feature(struct build_feature_devs_info *binfo,
> > + struct fpga_enum_dfl *dfl, resource_size_t ofst)
> > +{
> > + u64 v;
> > + u32 type;
> > +
> > + v = readq(dfl->ioaddr + ofst + DFH);
> > + type = FIELD_GET(DFH_TYPE, v);
> > +
> > + switch (type) {
> > + case DFH_TYPE_AFU:
> > + return parse_feature_afu(binfo, dfl, ofst);
> > + case DFH_TYPE_PRIVATE:
> > + return parse_feature_private(binfo, dfl, ofst);
> > + case DFH_TYPE_FIU:
> > + return parse_feature_fiu(binfo, dfl, ofst);
> > + default:
> > + dev_info(binfo->dev,
> > + "Feature Type %x is not supported.\n", type);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int parse_feature_list(struct build_feature_devs_info *binfo,
> > + struct fpga_enum_dfl *dfl)
> > +{
> > + void __iomem *start = dfl->ioaddr;
> > + void __iomem *end = dfl->ioaddr + dfl->len;
> > + int ret = 0;
> > + u32 ofst = 0;
> > + u64 v;
> > +
> > + /* walk through the device feature list via DFH's next DFH pointer. */
> > + for (; start < end; start += ofst) {
> > + if (end - start < DFH_SIZE) {
> > + dev_err(binfo->dev, "The region is too small to contain a feature.\n");
> > + return -EINVAL;
> > + }
> > +
> > + ret = parse_feature(binfo, dfl, start - dfl->ioaddr);
> > + if (ret)
> > + return ret;
> > +
> > + v = readq(start + DFH);
> > + ofst = FIELD_GET(DFH_NEXT_HDR_OFST, v);
> > +
> > + /* stop parsing if EOL(End of List) is set or offset is 0 */
> > + if ((v & DFH_EOL) || !ofst)
> > + break;
> > + }
> > +
> > + /* commit current feature device when reach the end of list */
> > + return build_info_commit_dev(binfo);
> > +}
> > +
> > +struct fpga_enum_info *fpga_enum_info_alloc(struct device *dev)
> > +{
> > + struct fpga_enum_info *info;
> > +
> > + get_device(dev);
> > +
> > + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> > + if (!info) {
> > + put_device(dev);
> > + return NULL;
> > + }
> > +
> > + info->dev = dev;
> > + INIT_LIST_HEAD(&info->dfls);
> > +
> > + return info;
> > +}
> > +EXPORT_SYMBOL_GPL(fpga_enum_info_alloc);
> > +
> > +void fpga_enum_info_free(struct fpga_enum_info *info)
> > +{
> > + struct fpga_enum_dfl *tmp, *dfl;
> > + struct device *dev;
> > +
> > + if (!info)
> > + return;
> > +
> > + dev = info->dev;
> > +
> > + /* remove all device feature lists in the list. */
> > + list_for_each_entry_safe(dfl, tmp, &info->dfls, node) {
> > + list_del(&dfl->node);
> > + devm_kfree(dev, dfl);
> > + }
> > +
> > + devm_kfree(dev, info);
> > + put_device(dev);
> > +}
> > +EXPORT_SYMBOL_GPL(fpga_enum_info_free);
> > +
> > +/**
> > + * fpga_enum_info_add_dfl - add info for a device feature list to fpga_enum_info
> > + *
> > + * @info: ptr to fpga_enum_info
> > + * @start: mmio resource address of the device feature list.
> > + * @len: mmio resource length of the device feature list.
> > + * @ioaddr: mapped mmio resource address of the device feature list.
> > + *
> > + * One FPGA device may have 1 or more Device Feature Lists (DFLs), use this
> > + * function to add information of each DFL to common data structure for next
> > + * step enumeration.
> > + *
> > + * Return: 0 on success, negative error code otherwise.
> > + */
> > +int fpga_enum_info_add_dfl(struct fpga_enum_info *info, resource_size_t start,
> > + resource_size_t len, void __iomem *ioaddr)
> > +{
> > + struct fpga_enum_dfl *dfl;
> > +
> > + dfl = devm_kzalloc(info->dev, sizeof(*dfl), GFP_KERNEL);
> > + if (!dfl)
> > + return -ENOMEM;
> > +
> > + dfl->start = start;
> > + dfl->len = len;
> > + dfl->ioaddr = ioaddr;
> > +
> > + list_add_tail(&dfl->node, &info->dfls);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(fpga_enum_info_add_dfl);
> > +
> > +static int remove_feature_dev(struct device *dev, void *data)
> > +{
> > + struct platform_device *pdev = to_platform_device(dev);
> > + enum fpga_id_type type = feature_dev_id_type(pdev);
> > + int id = pdev->id;
> > +
> > + platform_device_unregister(pdev);
> > +
> > + free_fpga_id(type, id);
> > +
> > + return 0;
> > +}
> > +
> > +static void remove_feature_devs(struct fpga_cdev *cdev)
> > +{
> > + device_for_each_child(&cdev->region.dev, NULL, remove_feature_dev);
> > +}
> > +
> > +/**
> > + * fpga_enumerate_feature_devs - enumerate feature devices
> > + * @info: information for enumeration.
> > + *
> > + * This function creates a container device (base FPGA region), enumerates
> > + * feature devices based on the enumeration info and creates platform devices
> > + * under the container device.
> > + *
> > + * Return: fpga_cdev struct on success, -errno on failure
> > + */
> > +struct fpga_cdev *fpga_enumerate_feature_devs(struct fpga_enum_info *info)
> > +{
> > + struct build_feature_devs_info *binfo;
> > + struct fpga_cdev *cdev;
> > + struct fpga_enum_dfl *dfl;
> > + int ret = 0;
> > +
> > + if (!info->dev)
> > + return ERR_PTR(-ENODEV);
> > +
> > + cdev = devm_kzalloc(info->dev, sizeof(*cdev), GFP_KERNEL);
> > + if (!cdev)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + cdev->parent = info->dev;
> > + mutex_init(&cdev->lock);
> > + INIT_LIST_HEAD(&cdev->port_dev_list);
> > + cdev->region.parent = info->dev;
> > +
> > + ret = fpga_region_register(&cdev->region);
> > + if (ret)
> > + goto free_cdev_exit;
> > +
> > + /* create and init build info for enumeration */
> > + binfo = devm_kzalloc(info->dev, sizeof(*binfo), GFP_KERNEL);
> > + if (!binfo) {
> > + ret = -ENOMEM;
> > + goto unregister_region_exit;
> > + }
> > +
> > + binfo->dev = info->dev;
> > + binfo->cdev = cdev;
> > +
> > + /*
> > + * start enumeration for all feature devices based on Device Feature
> > + * Lists.
> > + */
> > + list_for_each_entry(dfl, &info->dfls, node) {
> > + ret = parse_feature_list(binfo, dfl);
> > + if (ret) {
> > + remove_feature_devs(cdev);
> > + build_info_free(binfo);
> > + goto unregister_region_exit;
> > + }
> > + }
> > +
> > + build_info_free(binfo);
> > +
> > + return cdev;
> > +
> > +unregister_region_exit:
> > + fpga_region_unregister(&cdev->region);
> > +free_cdev_exit:
> > + devm_kfree(cdev->parent, cdev);
> > + return ERR_PTR(ret);
> > +}
> > +EXPORT_SYMBOL_GPL(fpga_enumerate_feature_devs);
> > +
> > +/**
> > + * fpga_remove_feature_devs - remove all feature devices
> > + * @cdev: fpga container device.
> > + *
> > + * Remove the container device and all feature devices under given container
> > + * devices.
> > + */
> > +void fpga_remove_feature_devs(struct fpga_cdev *cdev)
> > +{
> > + struct feature_platform_data *pdata, *ptmp;
> > +
> > + remove_feature_devs(cdev);
> > +
> > + mutex_lock(&cdev->lock);
> > + if (cdev->fme_dev) {
> > + /* the fme should be unregistered. */
> > + WARN_ON(device_is_registered(cdev->fme_dev));
> > + put_device(cdev->fme_dev);
> > + }
> > +
> > + list_for_each_entry_safe(pdata, ptmp, &cdev->port_dev_list, node) {
> > + struct platform_device *port_dev = pdata->dev;
> > +
> > + /* the port should be unregistered. */
> > + WARN_ON(device_is_registered(&port_dev->dev));
> > + list_del(&pdata->node);
> > + put_device(&port_dev->dev);
> > + }
> > + mutex_unlock(&cdev->lock);
> > +
> > + fpga_region_unregister(&cdev->region);
> > + devm_kfree(cdev->parent, cdev);
> > +}
> > +EXPORT_SYMBOL_GPL(fpga_remove_feature_devs);
> > +
> > +int fpga_port_id(struct platform_device *pdev)
> > +{
> > + void __iomem *base = get_feature_ioaddr_by_id(&pdev->dev,
> > + PORT_FEATURE_ID_HEADER);
> > +
> > + return FIELD_GET(PORT_CAP_PORT_NUM, readq(base + PORT_HDR_CAP));
> > +}
> > +EXPORT_SYMBOL_GPL(fpga_port_id);
> > +
> > +/**
> > + * __fpga_port_enable - enable a port
> > + * @pdev: port platform device.
> > + *
> > + * Enable Port by clear the port soft reset bit, which is set by default.
> > + * The User AFU is unable to respond to any MMIO access while in reset.
> > + * __fpga_port_enable function should only be used after __fpga_port_disable
> > + * function.
> > + */
> > +void __fpga_port_enable(struct platform_device *pdev)
> > +{
> > + struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > + void __iomem *base;
> > + u64 v;
> > +
> > + WARN_ON(!pdata->disable_count);
> > +
> > + if (--pdata->disable_count != 0)
> > + return;
> > +
> > + base = get_feature_ioaddr_by_id(&pdev->dev, PORT_FEATURE_ID_HEADER);
> > +
> > + /* Clear port soft reset */
> > + v = readq(base + PORT_HDR_CTRL);
> > + v &= ~PORT_CTRL_SFTRST;
> > + writeq(v, base + PORT_HDR_CTRL);
> > +}
> > +EXPORT_SYMBOL_GPL(__fpga_port_enable);
> > +
> > +#define RST_POLL_INVL 10 /* us */
> > +#define RST_POLL_TIMEOUT 1000 /* us */
> > +
> > +/**
> > + * __fpga_port_disable - disable a port
> > + * @pdev: port platform device.
> > + *
> > + * Disable Port by setting the port soft reset bit, it puts the port into
> > + * reset.
> > + */
> > +int __fpga_port_disable(struct platform_device *pdev)
> > +{
> > + struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > + void __iomem *base;
> > + u64 v;
> > +
> > + if (pdata->disable_count++ != 0)
> > + return 0;
> > +
> > + base = get_feature_ioaddr_by_id(&pdev->dev, PORT_FEATURE_ID_HEADER);
> > +
> > + /* Set port soft reset */
> > + v = readq(base + PORT_HDR_CTRL);
> > + v |= PORT_CTRL_SFTRST;
> > + writeq(v, base + PORT_HDR_CTRL);
> > +
> > + /*
> > + * HW sets ack bit to 1 when all outstanding requests have been drained
> > + * on this port and minimum soft reset pulse width has elapsed.
> > + * Driver polls port_soft_reset_ack to determine if reset done by HW.
> > + */
> > + if (readq_poll_timeout(base + PORT_HDR_CTRL, v, v & PORT_CTRL_SFTRST,
> > + RST_POLL_INVL, RST_POLL_TIMEOUT)) {
> > + dev_err(&pdev->dev, "timeout, fail to reset device\n");
> > + return -ETIMEDOUT;
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(__fpga_port_disable);
> > +
> > +static int __init dfl_fpga_init(void)
> > +{
> > + fpga_ids_init();
> > +
> > + return 0;
> > +}
> > +
> > +static void __exit dfl_fpga_exit(void)
> > +{
> > + fpga_ids_destroy();
> > +}
> > +
> > +module_init(dfl_fpga_init);
> > +module_exit(dfl_fpga_exit);
> > +
> > +MODULE_DESCRIPTION("FPGA Device Feature List (DFL) Support");
> > +MODULE_AUTHOR("Intel Corporation");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > new file mode 100644
> > index 0000000..22dcf73
> > --- /dev/null
> > +++ b/drivers/fpga/dfl.h
> > @@ -0,0 +1,345 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Driver Header File for FPGA Device Feature List (DFL) Support
> > + *
> > + * Copyright (C) 2017 Intel Corporation, Inc.
> > + *
> > + * Authors:
> > + * Kang Luwei <luwei.kang@intel.com>
> > + * Zhang Yi <yi.z.zhang@intel.com>
> > + * Wu Hao <hao.wu@intel.com>
> > + * Xiao Guangrong <guangrong.xiao@linux.intel.com>
> > + */
> > +
> > +#ifndef __FPGA_DFL_H
> > +#define __FPGA_DFL_H
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/delay.h>
> > +#include <linux/fs.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/io-64-nonatomic-lo-hi.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/uuid.h>
> > +#include <linux/fpga/fpga-region.h>
> > +
> > +/* maximum supported number of ports */
> > +#define MAX_FPGA_PORT_NUM 4
> > +/* plus one for fme device */
> > +#define MAX_FEATURE_DEV_NUM (MAX_FPGA_PORT_NUM + 1)
> > +
> > +/* Reserved 0x0 for Header Group Register and 0xff for AFU */
> > +#define FEATURE_ID_FIU_HEADER 0x0
> > +#define FEATURE_ID_AFU 0xff
> > +
> > +#define FME_FEATURE_ID_HEADER FEATURE_ID_FIU_HEADER
> > +#define FME_FEATURE_ID_THERMAL_MGMT 0x1
> > +#define FME_FEATURE_ID_POWER_MGMT 0x2
> > +#define FME_FEATURE_ID_GLOBAL_IPERF 0x3
> > +#define FME_FEATURE_ID_GLOBAL_ERR 0x4
> > +#define FME_FEATURE_ID_PR_MGMT 0x5
> > +#define FME_FEATURE_ID_HSSI 0x6
> > +#define FME_FEATURE_ID_GLOBAL_DPERF 0x7
> > +
> > +#define PORT_FEATURE_ID_HEADER FEATURE_ID_FIU_HEADER
> > +#define PORT_FEATURE_ID_AFU FEATURE_ID_AFU
> > +#define PORT_FEATURE_ID_ERROR 0x10
> > +#define PORT_FEATURE_ID_UMSG 0x11
> > +#define PORT_FEATURE_ID_UINT 0x12
> > +#define PORT_FEATURE_ID_STP 0x13
> > +
> > +/*
> > + * Device Feature Header Register Set
> > + *
> > + * For FIUs, they all have DFH + GUID + NEXT_AFU as common header registers.
> > + * For AFUs, they have DFH + GUID as common header registers.
> > + * For private features, they only have DFH register as common header.
> > + */
> > +#define DFH 0x0
> > +#define GUID_L 0x8
> > +#define GUID_H 0x10
> > +#define NEXT_AFU 0x18
> > +
> > +#define DFH_SIZE 0x8
> > +
> > +/* Device Feature Header Register Bitfield */
> > +#define DFH_ID GENMASK_ULL(11, 0) /* Feature ID */
> > +#define DFH_ID_FIU_FME 0
> > +#define DFH_ID_FIU_PORT 1
> > +#define DFH_REVISION GENMASK_ULL(15, 12) /* Feature revision */
> > +#define DFH_NEXT_HDR_OFST GENMASK_ULL(39, 16) /* Offset to next DFH */
> > +#define DFH_EOL BIT(40) /* End of list */
> > +#define DFH_TYPE GENMASK_ULL(63, 60) /* Feature type */
> > +#define DFH_TYPE_AFU 1
> > +#define DFH_TYPE_PRIVATE 3
> > +#define DFH_TYPE_FIU 4
> > +
> > +/* Next AFU Register Bitfield */
> > +#define NEXT_AFU_NEXT_DFH_OFST GENMASK_ULL(23, 0) /* Offset to next AFU */
> > +
> > +/* FME Header Register Set */
> > +#define FME_HDR_DFH DFH
> > +#define FME_HDR_GUID_L GUID_L
> > +#define FME_HDR_GUID_H GUID_H
> > +#define FME_HDR_NEXT_AFU NEXT_AFU
> > +#define FME_HDR_CAP 0x30
> > +#define FME_HDR_PORT_OFST(n) (0x38 + ((n) * 0x8))
> > +#define FME_HDR_BITSTREAM_ID 0x60
> > +#define FME_HDR_BITSTREAM_MD 0x68
> > +
> > +/* FME Fab Capability Register Bitfield */
> > +#define FME_CAP_FABRIC_VERID GENMASK_ULL(7, 0) /* Fabric version ID */
> > +#define FME_CAP_SOCKET_ID BIT(8) /* Socket ID */
> > +#define FME_CAP_PCIE0_LINK_AVL BIT(12) /* PCIE0 Link */
> > +#define FME_CAP_PCIE1_LINK_AVL BIT(13) /* PCIE1 Link */
> > +#define FME_CAP_COHR_LINK_AVL BIT(14) /* Coherent Link */
> > +#define FME_CAP_IOMMU_AVL BIT(16) /* IOMMU available */
> > +#define FME_CAP_NUM_PORTS GENMASK_ULL(19, 17) /* Number of ports */
> > +#define FME_CAP_ADDR_WIDTH GENMASK_ULL(29, 24) /* Address bus width */
> > +#define FME_CAP_CACHE_SIZE GENMASK_ULL(43, 32) /* cache size in KB */
> > +#define FME_CAP_CACHE_ASSOC GENMASK_ULL(47, 44) /* Associativity */
> > +
> > +/* FME Port Offset Register Bitfield */
> > +/* Offset to port device feature header */
> > +#define FME_PORT_OFST_DFH_OFST GENMASK_ULL(23, 0)
> > +/* PCI Bar ID for this port */
> > +#define FME_PORT_OFST_BAR_ID GENMASK_ULL(34, 32)
> > +/* AFU MMIO access permission. 1 - VF, 0 - PF. */
> > +#define FME_PORT_OFST_ACC_CTRL BIT(55)
> > +#define FME_PORT_OFST_ACC_PF 0
> > +#define FME_PORT_OFST_ACC_VF 1
> > +#define FME_PORT_OFST_IMP BIT(60)
> > +
> > +/* PORT Header Register Set */
> > +#define PORT_HDR_DFH DFH
> > +#define PORT_HDR_GUID_L GUID_L
> > +#define PORT_HDR_GUID_H GUID_H
> > +#define PORT_HDR_NEXT_AFU NEXT_AFU
> > +#define PORT_HDR_CAP 0x30
> > +#define PORT_HDR_CTRL 0x38
> > +
> > +/* Port Capability Register Bitfield */
> > +#define PORT_CAP_PORT_NUM GENMASK_ULL(1, 0) /* ID of this port */
> > +#define PORT_CAP_MMIO_SIZE GENMASK_ULL(23, 8) /* MMIO size in KB */
> > +#define PORT_CAP_SUPP_INT_NUM GENMASK_ULL(35, 32) /* Interrupts num */
> > +
> > +/* Port Control Register Bitfield */
> > +#define PORT_CTRL_SFTRST BIT(0) /* Port soft reset */
> > +/* Latency tolerance reporting. '1' >= 40us, '0' < 40us.*/
> > +#define PORT_CTRL_LATENCY BIT(2)
> > +#define PORT_CTRL_SFTRST_ACK BIT(4) /* HW ack for reset */
> > +
> > +/**
> > + * struct feature - sub feature of the feature devices
> > + *
> > + * @id: sub feature id.
> > + * @resource_index: each sub feature has one mmio resource for its registers.
> > + * this index is used to find its mmio resource from the
> > + * feature dev (platform device)'s reources.
> > + * @ioaddr: mapped mmio resource address.
> > + */
> > +struct feature {
> > + u64 id;
> > + int resource_index;
> > + void __iomem *ioaddr;
> > +};
> > +
> > +/**
> > + * struct feature_platform_data - platform data for feature devices
> > + *
> > + * @node: node to link feature devs to container device's port_dev_list.
> > + * @lock: mutex to protect platform data.
> > + * @dev: ptr to platform device linked with this platform data.
> > + * @disable_count: count for port disable.
> > + * @num: number for sub features.
> > + * @features: sub features of this feature dev.
> > + */
> > +struct feature_platform_data {
> > + struct list_head node;
> > + struct mutex lock;
> > + struct platform_device *dev;
> > + unsigned int disable_count;
> > +
> > + int num;
> > + struct feature features[0];
> > +};
> > +
> > +#define FPGA_FEATURE_DEV_FME "dfl-fme"
> > +#define FPGA_FEATURE_DEV_PORT "dfl-port"
> > +
> > +static inline int feature_platform_data_size(const int num)
> > +{
> > + return sizeof(struct feature_platform_data) +
> > + num * sizeof(struct feature);
> > +}
> > +
> > +int fpga_port_id(struct platform_device *pdev);
> > +
> > +static inline int fpga_port_check_id(struct platform_device *pdev,
> > + void *pport_id)
> > +{
> > + return fpga_port_id(pdev) == *(int *)pport_id;
> > +}
> > +
> > +void __fpga_port_enable(struct platform_device *pdev);
> > +int __fpga_port_disable(struct platform_device *pdev);
> > +
> > +static inline void fpga_port_enable(struct platform_device *pdev)
> > +{
> > + struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > +
> > + mutex_lock(&pdata->lock);
> > + __fpga_port_enable(pdev);
> > + mutex_unlock(&pdata->lock);
> > +}
> > +
> > +static inline int fpga_port_disable(struct platform_device *pdev)
> > +{
> > + struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > + int ret;
> > +
> > + mutex_lock(&pdata->lock);
> > + ret = __fpga_port_disable(pdev);
> > + mutex_unlock(&pdata->lock);
> > +
> > + return ret;
> > +}
> > +
> > +/*
> > + * This function resets the FPGA Port and its accelerator (AFU) by function
> > + * __fpga_port_disable and __fpga_port_enable (set port soft reset bit and
> > + * then clear it). Userspace can do Port reset at any time, e.g during DMA
> > + * or Partial Reconfiguration. But it should never cause any system level
> > + * issue, only functional failure (e.g DMA or PR operation failure) and be
> > + * recoverable from the failure.
> > + *
> > + * Note: the accelerator (AFU) is not accessible when its port is in reset
> > + * (disabled). Any attempts on MMIO access to AFU while in reset, will
> > + * result errors reported via port error reporting sub feature (if present).
> > + */
> > +static inline int __fpga_port_reset(struct platform_device *pdev)
> > +{
> > + int ret;
> > +
> > + ret = __fpga_port_disable(pdev);
> > + if (ret)
> > + return ret;
> > +
> > + __fpga_port_enable(pdev);
> > +
> > + return 0;
> > +}
> > +
> > +static inline int fpga_port_reset(struct platform_device *pdev)
> > +{
> > + struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > + int ret;
> > +
> > + mutex_lock(&pdata->lock);
> > + ret = __fpga_port_reset(pdev);
> > + mutex_unlock(&pdata->lock);
> > +
> > + return ret;
> > +}
>
> I'm still scratching my head about how the enumeration code also has
> code that handles resetting the PL in a FPGA region and
> enabling/disabling the bridge. We've discussed this before [1] and I
> know you've looked into it, I'm still trying to figure out how this
> can be made modular, so when someone needs to support a different port
> in the future, it isn't a complete rewrite.
>
> Speaking of resets, one way forward would be to create a reset
> controller for the port (and if possible move the port code to the
> bridge platform driver). The current linux-next repo adds support for
> reset lookups, so that reset controllers are supported for non-DT
> platforms [2].
>
> So the bridge driver would implement the enable/disable functions and
> create a reset controller, the fpga-region (or whoever else needs it)
> could look the reset controller and use the reset. By using the
> kernel reset framework, we don't have to have that piece of code
> shared around by having a reset function in a .h file. And it avoids
> adding extra dependencies between modules. Also, where necessary, I'd
> rather add functionality to the existing bridge/mgr/region frameworks,
> adding common interfaces at that level to allow reuse (like adding
> status to fpga-mgr). Ideally, this DFL framework would sit on top of
> mgr and bridge and allow those to be swapped out for reuse of the DFL
> framework on other devices. Also it will save future headaches as mgr
> or port implementations evolve.
Thanks a lot for the suggestion. I really really appreciate this.
Actually if we consider the virutalization case as I mentioned in [1] below,
that means AFU and its Port will be turned into a PCI VF and assigned (passed
through) to a virtual machine. There is no FME block on that PCI VF device,
(the FME is always kept in PCI PF device in the host) and currently the bridge
is created by FME module for PR functionatily. So in the guest virtual machine,
nobody creates the reset controller actually.
As I mentioned in [1], one possible method is, put these port reset functions to
AFU (Port) module, and share those functions with FME bridge module. I think
that will make the code in the common DFL framework a little more clean, but it
will introduce some module dependency here for sure, (e.g FME modules can't
finish PR without AFU (Port) Module loaded). But anyway it may be still
acceptable for users as all these modules could be loaded automatically. How do
you think? :)
Thanks
Hao
>
> Alan
>
> [1] https://lkml.org/lkml/2017/12/22/398
> [2] https://patchwork.kernel.org/patch/10247475/
>
> > +
> > +#define fpga_dev_for_each_feature(pdata, feature) \
> > + for ((feature) = (pdata)->features; \
> > + (feature) < (pdata)->features + (pdata)->num; (feature)++)
> > +
> > +static inline struct feature *get_feature_by_id(struct device *dev, u64 id)
> > +{
> > + struct feature_platform_data *pdata = dev_get_platdata(dev);
> > + struct feature *feature;
> > +
> > + fpga_dev_for_each_feature(pdata, feature)
> > + if (feature->id == id)
> > + return feature;
> > +
> > + return NULL;
> > +}
> > +
> > +static inline void __iomem *get_feature_ioaddr_by_id(struct device *dev, u64 id)
> > +{
> > + struct feature *feature = get_feature_by_id(dev, id);
> > +
> > + if (feature && feature->ioaddr)
> > + return feature->ioaddr;
> > +
> > + WARN_ON(1);
> > + return NULL;
> > +}
> > +
> > +static inline bool feature_is_fme(void __iomem *base)
> > +{
> > + u64 v = readq(base + DFH);
> > +
> > + return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> > + (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_FME);
> > +}
> > +
> > +static inline bool feature_is_port(void __iomem *base)
> > +{
> > + u64 v = readq(base + DFH);
> > +
> > + return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> > + (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_PORT);
> > +}
> > +
> > +/**
> > + * struct fpga_enum_info - FPGA enumeration information
> > + *
> > + * @dev: parent device.
> > + * @dfls: list of device feature lists.
> > + */
> > +struct fpga_enum_info {
> > + struct device *dev;
> > + struct list_head dfls;
> > +};
> > +
> > +/**
> > + * struct fpga_enum_dfl - FPGA enumeration device feature list information
> > + *
> > + * @start: base address of this device feature list.
> > + * @len: size of this device feature list.
> > + * @ioaddr: mapped base address of this device feature list.
> > + * @node: node in list of device feature lists.
> > + */
> > +struct fpga_enum_dfl {
> > + resource_size_t start;
> > + resource_size_t len;
> > +
> > + void __iomem *ioaddr;
> > +
> > + struct list_head node;
> > +};
> > +
> > +struct fpga_enum_info *fpga_enum_info_alloc(struct device *dev);
> > +int fpga_enum_info_add_dfl(struct fpga_enum_info *info, resource_size_t start,
> > + resource_size_t len, void __iomem *ioaddr);
> > +void fpga_enum_info_free(struct fpga_enum_info *info);
> > +
> > +/**
> > + * struct fpga_cdev - fpga container device
> > + *
> > + * @parent: parent device of this container device.
> > + * @region: base fpga region.
> > + * @fme_dev: FME feature device under this container device.
> > + * @lock: mutex lock to protect the port device list.
> > + * @port_dev_list: list of all port feature devices under this container device.
> > + */
> > +struct fpga_cdev {
> > + struct device *parent;
> > +
> > + struct fpga_region region;
> > +
> > + struct device *fme_dev;
> > +
> > + struct mutex lock; /* to protect the port device list */
> > + struct list_head port_dev_list;
> > +};
> > +
> > +struct fpga_cdev *fpga_enumerate_feature_devs(struct fpga_enum_info *info);
> > +void fpga_remove_feature_devs(struct fpga_cdev *cdev);
> > +
> > +#endif /* __FPGA_DFL_H */
> > --
> > 2.7.4
> >
^ permalink raw reply
* Re: [PATCH net-next] modules: allow modprobe load regular elf binaries
From: Luis R. Rodriguez @ 2018-03-23 2:47 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Alexei Starovoitov, Jessica Yu, Linus Torvalds, Mimi Zohar,
Djalal Harouni, David Miller, Kees Cook, Alexei Starovoitov,
Al Viro, Daniel Borkmann, Greg Kroah-Hartman, Network Development,
Linux Kernel Mailing List, kernel-team, Linux API, Michal Hocko,
Hannes Reinecke, werner
In-Reply-To: <CALCETrWFhTurv0gwxRcdLn4CD-1ySE=v4ZgvbsUYJqruZNMptQ@mail.gmail.com>
On Thu, Mar 22, 2018 at 3:15 PM, Andy Lutomirski <luto@kernel.org> wrote:
> All we need to do is to make sure that, if this is
> distributed as a module, that it's init routine doesn't wait for a
> long time, right?
Yeap.
Luis
^ permalink raw reply
* Re: [PATCH net-next] modules: allow modprobe load regular elf binaries
From: Alexei Starovoitov @ 2018-03-22 22:21 UTC (permalink / raw)
To: Andy Lutomirski, Luis R. Rodriguez
Cc: Jessica Yu, Linus Torvalds, Mimi Zohar, Djalal Harouni,
David Miller, Kees Cook, Alexei Starovoitov, Al Viro,
Daniel Borkmann, Greg Kroah-Hartman, Network Development,
Linux Kernel Mailing List, kernel-team, Linux API, Michal Hocko,
Hannes Reinecke, werner
In-Reply-To: <CALCETrWFhTurv0gwxRcdLn4CD-1ySE=v4ZgvbsUYJqruZNMptQ@mail.gmail.com>
On 3/22/18 3:15 PM, Andy Lutomirski wrote:
> On Thu, Mar 22, 2018 at 8:54 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>> If we can ensure that these usermode modules don't take *any time at all* on
>> their init *from the start*, it would be wonderful and we'd end up avoiding
>> some really odd corner case issues later.
>>
>
> I don't see why this issue needs to exist at all for the new stuff.
> After all, the new things aren't usermode modules per se. They're
> regular kernel code (modular or otherwise) that loads a usermode
> helper. All we need to do is to make sure that, if this is
> distributed as a module, that it's init routine doesn't wait for a
> long time, right?
I've implemented all of the previous suggestions and
now there are zero changes to kernel/module.c
I still need to finish tracpoint stuff first and polish umh code a bit
before sending new version.
Let's hold on this thread until then.
^ permalink raw reply
* Re: [PATCH net-next] modules: allow modprobe load regular elf binaries
From: Andy Lutomirski @ 2018-03-22 22:15 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Alexei Starovoitov, Jessica Yu, Linus Torvalds, Mimi Zohar,
Djalal Harouni, David Miller, Kees Cook, Alexei Starovoitov,
Al Viro, Daniel Borkmann, Greg Kroah-Hartman, Network Development,
Linux Kernel Mailing List, kernel-team, Linux API, Michal Hocko,
Hannes Reinecke, werner
In-Reply-To: <20180322205413.GA30543@wotan.suse.de>
On Thu, Mar 22, 2018 at 8:54 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> If we can ensure that these usermode modules don't take *any time at all* on
> their init *from the start*, it would be wonderful and we'd end up avoiding
> some really odd corner case issues later.
>
I don't see why this issue needs to exist at all for the new stuff.
After all, the new things aren't usermode modules per se. They're
regular kernel code (modular or otherwise) that loads a usermode
helper. All we need to do is to make sure that, if this is
distributed as a module, that it's init routine doesn't wait for a
long time, right?
^ permalink raw reply
* Re: [PATCH v4 04/24] fpga: add device feature list support
From: Alan Tull @ 2018-03-22 21:31 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <1518513893-4719-5-git-send-email-hao.wu@intel.com>
On Tue, Feb 13, 2018 at 3:24 AM, Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
> Device Feature List (DFL) defines a feature list structure that creates
> a link list of feature headers within the MMIO space to provide an
> extensible way of adding features. This patch introduces a kernel module
> to provide basic infrastructure to support FPGA devices which implement
> the Device Feature List.
>
> Usually there will be different features and their sub features linked into
> the DFL. This code provides common APIs for feature enumeration, it creates
> a container device (FPGA base region), walks through the DFLs and creates
> platform devices for feature devices (Currently it only supports two
> different feature devices, FPGA Management Engine (FME) and Port which
> the Accelerator Function Unit (AFU) connected to). In order to enumerate
> the DFLs, the common APIs required low level driver to provide necessary
> enumeration information (e.g address for each device feature list for
> given device) and fill it to the fpga_enum_info data structure. Please
> refer to below description for APIs added for enumeration.
>
> Functions for enumeration information preparation:
> *fpga_enum_info_alloc
> allocate enumeration information data structure.
>
> *fpga_enum_info_add_dfl
> add a device feature list to fpga_enum_info data structure.
>
> *fpga_enum_info_free
> free fpga_enum_info data structure and related resources.
>
> Functions for feature device enumeration:
> *fpga_enumerate_feature_devs
> enumerate feature devices and return container device.
>
> *fpga_remove_feature_devs
> remove feature devices under given container device.
This header doesn't say anything about the reset or bridge
functionality (fpga_port_enable/disable/reset) that's added here and
used elsewhere in the patch set.
>
> Signed-off-by: Tim Whisonant <tim.whisonant@intel.com>
> Signed-off-by: Enno Luebbers <enno.luebbers@intel.com>
> Signed-off-by: Shiva Rao <shiva.rao@intel.com>
> Signed-off-by: Christopher Rauer <christopher.rauer@intel.com>
> Signed-off-by: Zhang Yi <yi.z.zhang@intel.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
> ---
> v3: split from another patch.
> separate dfl enumeration code from original pcie driver.
> provide common data structures and APIs for enumeration.
> update device feature list parsing process according to latest hw.
> add dperf/iperf/hssi sub feature placeholder according to latest hw.
> remove build_info_add_sub_feature and other small functions.
> replace *_feature_num function with macro.
> remove writeq/readq.
> v4: fix SPDX license issue
> rename files to dfl.[ch], fix typo and add more comments.
> remove static feature_info tables for FME and Port.
> remove check on next_afu link list as only FIU has next_afu ptr.
> remove unused macro in header file.
> add more comments for functions.
> ---
> drivers/fpga/Kconfig | 16 +
> drivers/fpga/Makefile | 3 +
> drivers/fpga/dfl.c | 787 ++++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/fpga/dfl.h | 345 ++++++++++++++++++++++
> 4 files changed, 1151 insertions(+)
> create mode 100644 drivers/fpga/dfl.c
> create mode 100644 drivers/fpga/dfl.h
>
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index f47ef84..01ad31f 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -124,4 +124,20 @@ config OF_FPGA_REGION
> Support for loading FPGA images by applying a Device Tree
> overlay.
>
> +config FPGA_DFL
> + tristate "FPGA Device Feature List (DFL) support"
> + select FPGA_BRIDGE
> + select FPGA_REGION
> + help
> + Device Feature List (DFL) defines a feature list structure that
> + creates a link list of feature headers within the MMIO space
> + to provide an extensible way of adding features for FPGA.
> + Driver can walk through the feature headers to enumerate feature
> + devices (e.g FPGA Management Engine, Port and Accelerator
> + Function Unit) and their private features for target FPGA devices.
> +
> + Select this option to enable common support for Field-Programmable
> + Gate Array (FPGA) solutions which implement Device Feature List.
> + It provides enumeration APIs, and feature device infrastructure.
> +
> endif # FPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 3cb276a..c4c62b9 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -27,3 +27,6 @@ obj-$(CONFIG_XILINX_PR_DECOUPLER) += xilinx-pr-decoupler.o
> # High Level Interfaces
> obj-$(CONFIG_FPGA_REGION) += fpga-region.o
> obj-$(CONFIG_OF_FPGA_REGION) += of-fpga-region.o
> +
> +# FPGA Device Feature List Support
> +obj-$(CONFIG_FPGA_DFL) += dfl.o
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> new file mode 100644
> index 0000000..f50694e
> --- /dev/null
> +++ b/drivers/fpga/dfl.c
> @@ -0,0 +1,787 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for FPGA Device Feature List (DFL) Support
> + *
> + * Copyright (C) 2017 Intel Corporation, Inc.
> + *
> + * Authors:
> + * Kang Luwei <luwei.kang@intel.com>
> + * Zhang Yi <yi.z.zhang@intel.com>
> + * Wu Hao <hao.wu@intel.com>
> + * Xiao Guangrong <guangrong.xiao@linux.intel.com>
> + */
> +#include <linux/module.h>
> +
> +#include "dfl.h"
> +
> +static DEFINE_MUTEX(fpga_id_mutex);
> +
> +enum fpga_id_type {
> + FME_ID, /* fme id allocation and mapping */
> + PORT_ID, /* port id allocation and mapping */
> + FPGA_ID_MAX,
> +};
> +
> +/* it is protected by fpga_id_mutex */
> +static struct idr fpga_ids[FPGA_ID_MAX];
> +
> +static void fpga_ids_init(void)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(fpga_ids); i++)
> + idr_init(fpga_ids + i);
> +}
> +
> +static void fpga_ids_destroy(void)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(fpga_ids); i++)
> + idr_destroy(fpga_ids + i);
> +}
> +
> +static int alloc_fpga_id(enum fpga_id_type type, struct device *dev)
> +{
> + int id;
> +
> + WARN_ON(type >= FPGA_ID_MAX);
> + mutex_lock(&fpga_id_mutex);
> + id = idr_alloc(fpga_ids + type, dev, 0, 0, GFP_KERNEL);
> + mutex_unlock(&fpga_id_mutex);
> +
> + return id;
> +}
> +
> +static void free_fpga_id(enum fpga_id_type type, int id)
> +{
> + WARN_ON(type >= FPGA_ID_MAX);
> + mutex_lock(&fpga_id_mutex);
> + idr_remove(fpga_ids + type, id);
> + mutex_unlock(&fpga_id_mutex);
> +}
> +
> +static enum fpga_id_type feature_dev_id_type(struct platform_device *pdev)
> +{
> + if (!strcmp(pdev->name, FPGA_FEATURE_DEV_FME))
> + return FME_ID;
> +
> + if (!strcmp(pdev->name, FPGA_FEATURE_DEV_PORT))
> + return PORT_ID;
> +
> + WARN_ON(1);
> +
> + return FPGA_ID_MAX;
> +}
> +
> +/**
> + * struct build_feature_devs_info - info collected during feature dev build.
> + *
> + * @dev: device to enumerate.
> + * @cdev: the container device for all feature devices.
> + * @feature_dev: current feature device.
> + * @ioaddr: header register region address of feature device in enumeration.
> + * @sub_features: a sub features link list for feature device in enumeration.
> + * @feature_num: number of sub features for feature device in enumeration.
> + */
> +struct build_feature_devs_info {
> + struct device *dev;
> + struct fpga_cdev *cdev;
> + struct platform_device *feature_dev;
> + void __iomem *ioaddr;
> + struct list_head sub_features;
> + int feature_num;
> +};
> +
> +/**
> + * struct feature_info - sub feature info collected during feature dev build.
> + *
> + * @fid: id of this sub feature.
> + * @mmio_res: mmio resource of this sub feature.
> + * @ioaddr: mapped base address of mmio resource.
> + * @node: node in sub_features link list.
> + */
> +struct feature_info {
> + u64 fid;
> + struct resource mmio_res;
> + void __iomem *ioaddr;
> + struct list_head node;
> +};
> +
> +static void fpga_cdev_add_port_dev(struct fpga_cdev *cdev,
> + struct platform_device *port_pdev)
> +{
> + struct feature_platform_data *pdata = dev_get_platdata(&port_pdev->dev);
> +
> + mutex_lock(&cdev->lock);
> + list_add(&pdata->node, &cdev->port_dev_list);
> + get_device(&pdata->dev->dev);
> + mutex_unlock(&cdev->lock);
> +}
> +
> +/*
> + * register current feature device, it is called when we need to switch to
> + * another feature parsing or we have parsed all features on given device
> + * feature list.
> + */
> +static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> +{
> + struct platform_device *fdev = binfo->feature_dev;
> + struct feature_platform_data *pdata;
> + struct feature_info *finfo, *p;
> + int ret, index = 0;
> +
> + if (!fdev)
> + return 0;
> +
> + /*
> + * we do not need to care for the memory which is associated with
> + * the platform device. After calling platform_device_unregister(),
> + * it will be automatically freed by device's release() callback,
> + * platform_device_release().
> + */
> + pdata = kzalloc(feature_platform_data_size(binfo->feature_num),
> + GFP_KERNEL);
> + if (pdata) {
> + pdata->dev = fdev;
> + pdata->num = binfo->feature_num;
> + mutex_init(&pdata->lock);
> + } else {
> + return -ENOMEM;
> + }
> +
> + /*
> + * the count should be initialized to 0 to make sure
> + *__fpga_port_enable() following __fpga_port_disable()
> + * works properly for port device.
> + * and it should always be 0 for fme device.
> + */
> + WARN_ON(pdata->disable_count);
> +
> + fdev->dev.platform_data = pdata;
> +
> + /* each sub feature has one MMIO resource */
> + fdev->num_resources = binfo->feature_num;
> + fdev->resource = kcalloc(binfo->feature_num, sizeof(*fdev->resource),
> + GFP_KERNEL);
> + if (!fdev->resource)
> + return -ENOMEM;
> +
> + /* fill features and resource information for feature dev */
> + list_for_each_entry_safe(finfo, p, &binfo->sub_features, node) {
> + struct feature *feature = &pdata->features[index];
> +
> + /* save resource information for each feature */
> + feature->id = finfo->fid;
> + feature->resource_index = index;
> + feature->ioaddr = finfo->ioaddr;
> + fdev->resource[index++] = finfo->mmio_res;
> +
> + list_del(&finfo->node);
> + kfree(finfo);
> + }
> +
> + ret = platform_device_add(binfo->feature_dev);
> + if (!ret) {
> + if (feature_dev_id_type(binfo->feature_dev) == PORT_ID)
> + fpga_cdev_add_port_dev(binfo->cdev, binfo->feature_dev);
> + else
> + binfo->cdev->fme_dev =
> + get_device(&binfo->feature_dev->dev);
> + /*
> + * reset it to avoid build_info_free() freeing their resource.
> + *
> + * The resource of successfully registered feature devices
> + * will be freed by platform_device_unregister(). See the
> + * comments in build_info_create_dev().
> + */
> + binfo->feature_dev = NULL;
> + }
> +
> + return ret;
> +}
> +
> +static int
> +build_info_create_dev(struct build_feature_devs_info *binfo,
> + enum fpga_id_type type, const char *name,
> + void __iomem *ioaddr)
> +{
> + struct platform_device *fdev;
> + int ret;
> +
> + /* we will create a new device, commit current device first */
> + ret = build_info_commit_dev(binfo);
> + if (ret)
> + return ret;
> +
> + /*
> + * we use -ENODEV as the initialization indicator which indicates
> + * whether the id need to be reclaimed
> + */
> + fdev = platform_device_alloc(name, -ENODEV);
> + if (!fdev)
> + return -ENOMEM;
> +
> + binfo->feature_dev = fdev;
> + binfo->feature_num = 0;
> + binfo->ioaddr = ioaddr;
> + INIT_LIST_HEAD(&binfo->sub_features);
> +
> + fdev->id = alloc_fpga_id(type, &fdev->dev);
> + if (fdev->id < 0)
> + return fdev->id;
> +
> + fdev->dev.parent = &binfo->cdev->region.dev;
> +
> + return 0;
> +}
> +
> +static void build_info_free(struct build_feature_devs_info *binfo)
> +{
> + struct feature_info *finfo, *p;
> +
> + /*
> + * it is a valid id, free it. See comments in
> + * build_info_create_dev()
> + */
> + if (binfo->feature_dev && binfo->feature_dev->id >= 0) {
> + free_fpga_id(feature_dev_id_type(binfo->feature_dev),
> + binfo->feature_dev->id);
> +
> + list_for_each_entry_safe(finfo, p, &binfo->sub_features, node) {
> + list_del(&finfo->node);
> + kfree(finfo);
> + }
> + }
> +
> + platform_device_put(binfo->feature_dev);
> +
> + devm_kfree(binfo->dev, binfo);
> +}
> +
> +static inline u32 feature_size(void __iomem *start)
> +{
> + u64 v = readq(start + DFH);
> + u32 ofst = FIELD_GET(DFH_NEXT_HDR_OFST, v);
> + /* workaround for private features with invalid size, use 4K instead */
> + return ofst ? ofst : 4096;
> +}
> +
> +static u64 feature_id(void __iomem *start)
> +{
> + u64 v = readq(start + DFH);
> + u16 id = FIELD_GET(DFH_ID, v);
> + u8 type = FIELD_GET(DFH_TYPE, v);
> +
> + if (type == DFH_TYPE_FIU)
> + return FEATURE_ID_FIU_HEADER;
> + else if (type == DFH_TYPE_PRIVATE)
> + return id;
> + else if (type == DFH_TYPE_AFU)
> + return FEATURE_ID_AFU;
> +
> + WARN_ON(1);
> + return 0;
> +}
> +
> +/*
> + * when create sub feature instances, for private features, it doesn't need
> + * to provide resource size and feature id as they could be read from DFH
> + * register. For afu sub feature, its register region only contains user
> + * defined registers, so never trust any information from it, just use the
> + * resource size information provided by its parent FIU.
> + */
> +static int
> +create_feature_instance(struct build_feature_devs_info *binfo,
> + struct fpga_enum_dfl *dfl, resource_size_t ofst,
> + resource_size_t size, u64 fid)
> +{
> + struct feature_info *finfo;
> +
> + /* read feature size and id if inputs are invalid */
> + size = size ? size : feature_size(dfl->ioaddr + ofst);
> + fid = fid ? fid : feature_id(dfl->ioaddr + ofst);
> +
> + if (dfl->len - ofst < size)
> + return -EINVAL;
> +
> + finfo = kzalloc(sizeof(*finfo), GFP_KERNEL);
> + if (!finfo)
> + return -ENOMEM;
> +
> + finfo->fid = fid;
> + finfo->mmio_res.start = dfl->start + ofst;
> + finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> + finfo->mmio_res.flags = IORESOURCE_MEM;
> + finfo->ioaddr = dfl->ioaddr + ofst;
> +
> + list_add_tail(&finfo->node, &binfo->sub_features);
> + binfo->feature_num++;
> +
> + return 0;
> +}
> +
> +static int parse_feature_fme(struct build_feature_devs_info *binfo,
> + struct fpga_enum_dfl *dfl, resource_size_t ofst)
> +{
> + int ret;
> +
> + ret = build_info_create_dev(binfo, FME_ID, FPGA_FEATURE_DEV_FME,
> + dfl->ioaddr + ofst);
> + if (ret)
> + return ret;
> +
> + return create_feature_instance(binfo, dfl, ofst, 0, 0);
> +}
> +
> +static int parse_feature_port(struct build_feature_devs_info *binfo,
> + struct fpga_enum_dfl *dfl,
> + resource_size_t ofst)
> +{
> + int ret;
> +
> + ret = build_info_create_dev(binfo, PORT_ID, FPGA_FEATURE_DEV_PORT,
> + dfl->ioaddr + ofst);
> + if (ret)
> + return ret;
> +
> + return create_feature_instance(binfo, dfl, ofst, 0, 0);
> +}
> +
> +static int parse_feature_port_afu(struct build_feature_devs_info *binfo,
> + struct fpga_enum_dfl *dfl,
> + resource_size_t ofst)
> +{
> + u64 v = readq(binfo->ioaddr + PORT_HDR_CAP);
> + u32 size = FIELD_GET(PORT_CAP_MMIO_SIZE, v) << 10;
> +
> + WARN_ON(!size);
> +
> + return create_feature_instance(binfo, dfl, ofst, size, FEATURE_ID_AFU);
> +}
> +
> +static int parse_feature_afu(struct build_feature_devs_info *binfo,
> + struct fpga_enum_dfl *dfl,
> + resource_size_t ofst)
> +{
> + if (!binfo->feature_dev) {
> + dev_err(binfo->dev, "this AFU does not belong to any FIU.\n");
> + return -EINVAL;
> + }
> +
> + switch (feature_dev_id_type(binfo->feature_dev)) {
> + case PORT_ID:
> + return parse_feature_port_afu(binfo, dfl, ofst);
> + default:
> + dev_info(binfo->dev, "AFU belonging to FIU %s is not supported yet.\n",
> + binfo->feature_dev->name);
> + }
> +
> + return 0;
> +}
> +
> +static int parse_feature_fiu(struct build_feature_devs_info *binfo,
> + struct fpga_enum_dfl *dfl,
> + resource_size_t ofst)
> +{
> + u32 id, offset;
> + u64 v;
> + int ret = 0;
> +
> + v = readq(dfl->ioaddr + ofst + DFH);
> + id = FIELD_GET(DFH_ID, v);
> +
> + switch (id) {
> + case DFH_ID_FIU_FME:
> + ret = parse_feature_fme(binfo, dfl, ofst);
> + break;
> + case DFH_ID_FIU_PORT:
> + ret = parse_feature_port(binfo, dfl, ofst);
> + break;
> + default:
> + dev_info(binfo->dev, "FIU TYPE %d is not supported yet.\n",
> + id);
> + }
> +
> + if (ret)
> + return ret;
> +
> + /* Find and parse FIU's child AFU via its NEXT_AFU register */
> + v = readq(dfl->ioaddr + ofst + NEXT_AFU);
> +
> + offset = FIELD_GET(NEXT_AFU_NEXT_DFH_OFST, v);
> + if (offset)
> + return parse_feature_afu(binfo, dfl, ofst + offset);
> +
> + dev_dbg(binfo->dev, "No AFUs detected on FIU %d\n", id);
> +
> + return ret;
> +}
> +
> +static int parse_feature_private(struct build_feature_devs_info *binfo,
> + struct fpga_enum_dfl *dfl,
> + resource_size_t ofst)
> +{
> + if (!binfo->feature_dev) {
> + dev_err(binfo->dev, "the private feature %llx does not belong to any AFU.\n",
> + (unsigned long long)feature_id(dfl->ioaddr + ofst));
> + return -EINVAL;
> + }
> +
> + return create_feature_instance(binfo, dfl, ofst, 0, 0);
> +}
> +
> +/**
> + * parse_feature - parse a feature on given device feature list
> + *
> + * @binfo: build feature devices information.
> + * @dfl: device feature list to parse
> + * @ofst: offset to feature header on this device feature list
> + */
> +static int parse_feature(struct build_feature_devs_info *binfo,
> + struct fpga_enum_dfl *dfl, resource_size_t ofst)
> +{
> + u64 v;
> + u32 type;
> +
> + v = readq(dfl->ioaddr + ofst + DFH);
> + type = FIELD_GET(DFH_TYPE, v);
> +
> + switch (type) {
> + case DFH_TYPE_AFU:
> + return parse_feature_afu(binfo, dfl, ofst);
> + case DFH_TYPE_PRIVATE:
> + return parse_feature_private(binfo, dfl, ofst);
> + case DFH_TYPE_FIU:
> + return parse_feature_fiu(binfo, dfl, ofst);
> + default:
> + dev_info(binfo->dev,
> + "Feature Type %x is not supported.\n", type);
> + }
> +
> + return 0;
> +}
> +
> +static int parse_feature_list(struct build_feature_devs_info *binfo,
> + struct fpga_enum_dfl *dfl)
> +{
> + void __iomem *start = dfl->ioaddr;
> + void __iomem *end = dfl->ioaddr + dfl->len;
> + int ret = 0;
> + u32 ofst = 0;
> + u64 v;
> +
> + /* walk through the device feature list via DFH's next DFH pointer. */
> + for (; start < end; start += ofst) {
> + if (end - start < DFH_SIZE) {
> + dev_err(binfo->dev, "The region is too small to contain a feature.\n");
> + return -EINVAL;
> + }
> +
> + ret = parse_feature(binfo, dfl, start - dfl->ioaddr);
> + if (ret)
> + return ret;
> +
> + v = readq(start + DFH);
> + ofst = FIELD_GET(DFH_NEXT_HDR_OFST, v);
> +
> + /* stop parsing if EOL(End of List) is set or offset is 0 */
> + if ((v & DFH_EOL) || !ofst)
> + break;
> + }
> +
> + /* commit current feature device when reach the end of list */
> + return build_info_commit_dev(binfo);
> +}
> +
> +struct fpga_enum_info *fpga_enum_info_alloc(struct device *dev)
> +{
> + struct fpga_enum_info *info;
> +
> + get_device(dev);
> +
> + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> + if (!info) {
> + put_device(dev);
> + return NULL;
> + }
> +
> + info->dev = dev;
> + INIT_LIST_HEAD(&info->dfls);
> +
> + return info;
> +}
> +EXPORT_SYMBOL_GPL(fpga_enum_info_alloc);
> +
> +void fpga_enum_info_free(struct fpga_enum_info *info)
> +{
> + struct fpga_enum_dfl *tmp, *dfl;
> + struct device *dev;
> +
> + if (!info)
> + return;
> +
> + dev = info->dev;
> +
> + /* remove all device feature lists in the list. */
> + list_for_each_entry_safe(dfl, tmp, &info->dfls, node) {
> + list_del(&dfl->node);
> + devm_kfree(dev, dfl);
> + }
> +
> + devm_kfree(dev, info);
> + put_device(dev);
> +}
> +EXPORT_SYMBOL_GPL(fpga_enum_info_free);
> +
> +/**
> + * fpga_enum_info_add_dfl - add info for a device feature list to fpga_enum_info
> + *
> + * @info: ptr to fpga_enum_info
> + * @start: mmio resource address of the device feature list.
> + * @len: mmio resource length of the device feature list.
> + * @ioaddr: mapped mmio resource address of the device feature list.
> + *
> + * One FPGA device may have 1 or more Device Feature Lists (DFLs), use this
> + * function to add information of each DFL to common data structure for next
> + * step enumeration.
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +int fpga_enum_info_add_dfl(struct fpga_enum_info *info, resource_size_t start,
> + resource_size_t len, void __iomem *ioaddr)
> +{
> + struct fpga_enum_dfl *dfl;
> +
> + dfl = devm_kzalloc(info->dev, sizeof(*dfl), GFP_KERNEL);
> + if (!dfl)
> + return -ENOMEM;
> +
> + dfl->start = start;
> + dfl->len = len;
> + dfl->ioaddr = ioaddr;
> +
> + list_add_tail(&dfl->node, &info->dfls);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(fpga_enum_info_add_dfl);
> +
> +static int remove_feature_dev(struct device *dev, void *data)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + enum fpga_id_type type = feature_dev_id_type(pdev);
> + int id = pdev->id;
> +
> + platform_device_unregister(pdev);
> +
> + free_fpga_id(type, id);
> +
> + return 0;
> +}
> +
> +static void remove_feature_devs(struct fpga_cdev *cdev)
> +{
> + device_for_each_child(&cdev->region.dev, NULL, remove_feature_dev);
> +}
> +
> +/**
> + * fpga_enumerate_feature_devs - enumerate feature devices
> + * @info: information for enumeration.
> + *
> + * This function creates a container device (base FPGA region), enumerates
> + * feature devices based on the enumeration info and creates platform devices
> + * under the container device.
> + *
> + * Return: fpga_cdev struct on success, -errno on failure
> + */
> +struct fpga_cdev *fpga_enumerate_feature_devs(struct fpga_enum_info *info)
> +{
> + struct build_feature_devs_info *binfo;
> + struct fpga_cdev *cdev;
> + struct fpga_enum_dfl *dfl;
> + int ret = 0;
> +
> + if (!info->dev)
> + return ERR_PTR(-ENODEV);
> +
> + cdev = devm_kzalloc(info->dev, sizeof(*cdev), GFP_KERNEL);
> + if (!cdev)
> + return ERR_PTR(-ENOMEM);
> +
> + cdev->parent = info->dev;
> + mutex_init(&cdev->lock);
> + INIT_LIST_HEAD(&cdev->port_dev_list);
> + cdev->region.parent = info->dev;
> +
> + ret = fpga_region_register(&cdev->region);
> + if (ret)
> + goto free_cdev_exit;
> +
> + /* create and init build info for enumeration */
> + binfo = devm_kzalloc(info->dev, sizeof(*binfo), GFP_KERNEL);
> + if (!binfo) {
> + ret = -ENOMEM;
> + goto unregister_region_exit;
> + }
> +
> + binfo->dev = info->dev;
> + binfo->cdev = cdev;
> +
> + /*
> + * start enumeration for all feature devices based on Device Feature
> + * Lists.
> + */
> + list_for_each_entry(dfl, &info->dfls, node) {
> + ret = parse_feature_list(binfo, dfl);
> + if (ret) {
> + remove_feature_devs(cdev);
> + build_info_free(binfo);
> + goto unregister_region_exit;
> + }
> + }
> +
> + build_info_free(binfo);
> +
> + return cdev;
> +
> +unregister_region_exit:
> + fpga_region_unregister(&cdev->region);
> +free_cdev_exit:
> + devm_kfree(cdev->parent, cdev);
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(fpga_enumerate_feature_devs);
> +
> +/**
> + * fpga_remove_feature_devs - remove all feature devices
> + * @cdev: fpga container device.
> + *
> + * Remove the container device and all feature devices under given container
> + * devices.
> + */
> +void fpga_remove_feature_devs(struct fpga_cdev *cdev)
> +{
> + struct feature_platform_data *pdata, *ptmp;
> +
> + remove_feature_devs(cdev);
> +
> + mutex_lock(&cdev->lock);
> + if (cdev->fme_dev) {
> + /* the fme should be unregistered. */
> + WARN_ON(device_is_registered(cdev->fme_dev));
> + put_device(cdev->fme_dev);
> + }
> +
> + list_for_each_entry_safe(pdata, ptmp, &cdev->port_dev_list, node) {
> + struct platform_device *port_dev = pdata->dev;
> +
> + /* the port should be unregistered. */
> + WARN_ON(device_is_registered(&port_dev->dev));
> + list_del(&pdata->node);
> + put_device(&port_dev->dev);
> + }
> + mutex_unlock(&cdev->lock);
> +
> + fpga_region_unregister(&cdev->region);
> + devm_kfree(cdev->parent, cdev);
> +}
> +EXPORT_SYMBOL_GPL(fpga_remove_feature_devs);
> +
> +int fpga_port_id(struct platform_device *pdev)
> +{
> + void __iomem *base = get_feature_ioaddr_by_id(&pdev->dev,
> + PORT_FEATURE_ID_HEADER);
> +
> + return FIELD_GET(PORT_CAP_PORT_NUM, readq(base + PORT_HDR_CAP));
> +}
> +EXPORT_SYMBOL_GPL(fpga_port_id);
> +
> +/**
> + * __fpga_port_enable - enable a port
> + * @pdev: port platform device.
> + *
> + * Enable Port by clear the port soft reset bit, which is set by default.
> + * The User AFU is unable to respond to any MMIO access while in reset.
> + * __fpga_port_enable function should only be used after __fpga_port_disable
> + * function.
> + */
> +void __fpga_port_enable(struct platform_device *pdev)
> +{
> + struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> + void __iomem *base;
> + u64 v;
> +
> + WARN_ON(!pdata->disable_count);
> +
> + if (--pdata->disable_count != 0)
> + return;
> +
> + base = get_feature_ioaddr_by_id(&pdev->dev, PORT_FEATURE_ID_HEADER);
> +
> + /* Clear port soft reset */
> + v = readq(base + PORT_HDR_CTRL);
> + v &= ~PORT_CTRL_SFTRST;
> + writeq(v, base + PORT_HDR_CTRL);
> +}
> +EXPORT_SYMBOL_GPL(__fpga_port_enable);
> +
> +#define RST_POLL_INVL 10 /* us */
> +#define RST_POLL_TIMEOUT 1000 /* us */
> +
> +/**
> + * __fpga_port_disable - disable a port
> + * @pdev: port platform device.
> + *
> + * Disable Port by setting the port soft reset bit, it puts the port into
> + * reset.
> + */
> +int __fpga_port_disable(struct platform_device *pdev)
> +{
> + struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> + void __iomem *base;
> + u64 v;
> +
> + if (pdata->disable_count++ != 0)
> + return 0;
> +
> + base = get_feature_ioaddr_by_id(&pdev->dev, PORT_FEATURE_ID_HEADER);
> +
> + /* Set port soft reset */
> + v = readq(base + PORT_HDR_CTRL);
> + v |= PORT_CTRL_SFTRST;
> + writeq(v, base + PORT_HDR_CTRL);
> +
> + /*
> + * HW sets ack bit to 1 when all outstanding requests have been drained
> + * on this port and minimum soft reset pulse width has elapsed.
> + * Driver polls port_soft_reset_ack to determine if reset done by HW.
> + */
> + if (readq_poll_timeout(base + PORT_HDR_CTRL, v, v & PORT_CTRL_SFTRST,
> + RST_POLL_INVL, RST_POLL_TIMEOUT)) {
> + dev_err(&pdev->dev, "timeout, fail to reset device\n");
> + return -ETIMEDOUT;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(__fpga_port_disable);
> +
> +static int __init dfl_fpga_init(void)
> +{
> + fpga_ids_init();
> +
> + return 0;
> +}
> +
> +static void __exit dfl_fpga_exit(void)
> +{
> + fpga_ids_destroy();
> +}
> +
> +module_init(dfl_fpga_init);
> +module_exit(dfl_fpga_exit);
> +
> +MODULE_DESCRIPTION("FPGA Device Feature List (DFL) Support");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> new file mode 100644
> index 0000000..22dcf73
> --- /dev/null
> +++ b/drivers/fpga/dfl.h
> @@ -0,0 +1,345 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Driver Header File for FPGA Device Feature List (DFL) Support
> + *
> + * Copyright (C) 2017 Intel Corporation, Inc.
> + *
> + * Authors:
> + * Kang Luwei <luwei.kang@intel.com>
> + * Zhang Yi <yi.z.zhang@intel.com>
> + * Wu Hao <hao.wu@intel.com>
> + * Xiao Guangrong <guangrong.xiao@linux.intel.com>
> + */
> +
> +#ifndef __FPGA_DFL_H
> +#define __FPGA_DFL_H
> +
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <linux/iopoll.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/uuid.h>
> +#include <linux/fpga/fpga-region.h>
> +
> +/* maximum supported number of ports */
> +#define MAX_FPGA_PORT_NUM 4
> +/* plus one for fme device */
> +#define MAX_FEATURE_DEV_NUM (MAX_FPGA_PORT_NUM + 1)
> +
> +/* Reserved 0x0 for Header Group Register and 0xff for AFU */
> +#define FEATURE_ID_FIU_HEADER 0x0
> +#define FEATURE_ID_AFU 0xff
> +
> +#define FME_FEATURE_ID_HEADER FEATURE_ID_FIU_HEADER
> +#define FME_FEATURE_ID_THERMAL_MGMT 0x1
> +#define FME_FEATURE_ID_POWER_MGMT 0x2
> +#define FME_FEATURE_ID_GLOBAL_IPERF 0x3
> +#define FME_FEATURE_ID_GLOBAL_ERR 0x4
> +#define FME_FEATURE_ID_PR_MGMT 0x5
> +#define FME_FEATURE_ID_HSSI 0x6
> +#define FME_FEATURE_ID_GLOBAL_DPERF 0x7
> +
> +#define PORT_FEATURE_ID_HEADER FEATURE_ID_FIU_HEADER
> +#define PORT_FEATURE_ID_AFU FEATURE_ID_AFU
> +#define PORT_FEATURE_ID_ERROR 0x10
> +#define PORT_FEATURE_ID_UMSG 0x11
> +#define PORT_FEATURE_ID_UINT 0x12
> +#define PORT_FEATURE_ID_STP 0x13
> +
> +/*
> + * Device Feature Header Register Set
> + *
> + * For FIUs, they all have DFH + GUID + NEXT_AFU as common header registers.
> + * For AFUs, they have DFH + GUID as common header registers.
> + * For private features, they only have DFH register as common header.
> + */
> +#define DFH 0x0
> +#define GUID_L 0x8
> +#define GUID_H 0x10
> +#define NEXT_AFU 0x18
> +
> +#define DFH_SIZE 0x8
> +
> +/* Device Feature Header Register Bitfield */
> +#define DFH_ID GENMASK_ULL(11, 0) /* Feature ID */
> +#define DFH_ID_FIU_FME 0
> +#define DFH_ID_FIU_PORT 1
> +#define DFH_REVISION GENMASK_ULL(15, 12) /* Feature revision */
> +#define DFH_NEXT_HDR_OFST GENMASK_ULL(39, 16) /* Offset to next DFH */
> +#define DFH_EOL BIT(40) /* End of list */
> +#define DFH_TYPE GENMASK_ULL(63, 60) /* Feature type */
> +#define DFH_TYPE_AFU 1
> +#define DFH_TYPE_PRIVATE 3
> +#define DFH_TYPE_FIU 4
> +
> +/* Next AFU Register Bitfield */
> +#define NEXT_AFU_NEXT_DFH_OFST GENMASK_ULL(23, 0) /* Offset to next AFU */
> +
> +/* FME Header Register Set */
> +#define FME_HDR_DFH DFH
> +#define FME_HDR_GUID_L GUID_L
> +#define FME_HDR_GUID_H GUID_H
> +#define FME_HDR_NEXT_AFU NEXT_AFU
> +#define FME_HDR_CAP 0x30
> +#define FME_HDR_PORT_OFST(n) (0x38 + ((n) * 0x8))
> +#define FME_HDR_BITSTREAM_ID 0x60
> +#define FME_HDR_BITSTREAM_MD 0x68
> +
> +/* FME Fab Capability Register Bitfield */
> +#define FME_CAP_FABRIC_VERID GENMASK_ULL(7, 0) /* Fabric version ID */
> +#define FME_CAP_SOCKET_ID BIT(8) /* Socket ID */
> +#define FME_CAP_PCIE0_LINK_AVL BIT(12) /* PCIE0 Link */
> +#define FME_CAP_PCIE1_LINK_AVL BIT(13) /* PCIE1 Link */
> +#define FME_CAP_COHR_LINK_AVL BIT(14) /* Coherent Link */
> +#define FME_CAP_IOMMU_AVL BIT(16) /* IOMMU available */
> +#define FME_CAP_NUM_PORTS GENMASK_ULL(19, 17) /* Number of ports */
> +#define FME_CAP_ADDR_WIDTH GENMASK_ULL(29, 24) /* Address bus width */
> +#define FME_CAP_CACHE_SIZE GENMASK_ULL(43, 32) /* cache size in KB */
> +#define FME_CAP_CACHE_ASSOC GENMASK_ULL(47, 44) /* Associativity */
> +
> +/* FME Port Offset Register Bitfield */
> +/* Offset to port device feature header */
> +#define FME_PORT_OFST_DFH_OFST GENMASK_ULL(23, 0)
> +/* PCI Bar ID for this port */
> +#define FME_PORT_OFST_BAR_ID GENMASK_ULL(34, 32)
> +/* AFU MMIO access permission. 1 - VF, 0 - PF. */
> +#define FME_PORT_OFST_ACC_CTRL BIT(55)
> +#define FME_PORT_OFST_ACC_PF 0
> +#define FME_PORT_OFST_ACC_VF 1
> +#define FME_PORT_OFST_IMP BIT(60)
> +
> +/* PORT Header Register Set */
> +#define PORT_HDR_DFH DFH
> +#define PORT_HDR_GUID_L GUID_L
> +#define PORT_HDR_GUID_H GUID_H
> +#define PORT_HDR_NEXT_AFU NEXT_AFU
> +#define PORT_HDR_CAP 0x30
> +#define PORT_HDR_CTRL 0x38
> +
> +/* Port Capability Register Bitfield */
> +#define PORT_CAP_PORT_NUM GENMASK_ULL(1, 0) /* ID of this port */
> +#define PORT_CAP_MMIO_SIZE GENMASK_ULL(23, 8) /* MMIO size in KB */
> +#define PORT_CAP_SUPP_INT_NUM GENMASK_ULL(35, 32) /* Interrupts num */
> +
> +/* Port Control Register Bitfield */
> +#define PORT_CTRL_SFTRST BIT(0) /* Port soft reset */
> +/* Latency tolerance reporting. '1' >= 40us, '0' < 40us.*/
> +#define PORT_CTRL_LATENCY BIT(2)
> +#define PORT_CTRL_SFTRST_ACK BIT(4) /* HW ack for reset */
> +
> +/**
> + * struct feature - sub feature of the feature devices
> + *
> + * @id: sub feature id.
> + * @resource_index: each sub feature has one mmio resource for its registers.
> + * this index is used to find its mmio resource from the
> + * feature dev (platform device)'s reources.
> + * @ioaddr: mapped mmio resource address.
> + */
> +struct feature {
> + u64 id;
> + int resource_index;
> + void __iomem *ioaddr;
> +};
> +
> +/**
> + * struct feature_platform_data - platform data for feature devices
> + *
> + * @node: node to link feature devs to container device's port_dev_list.
> + * @lock: mutex to protect platform data.
> + * @dev: ptr to platform device linked with this platform data.
> + * @disable_count: count for port disable.
> + * @num: number for sub features.
> + * @features: sub features of this feature dev.
> + */
> +struct feature_platform_data {
> + struct list_head node;
> + struct mutex lock;
> + struct platform_device *dev;
> + unsigned int disable_count;
> +
> + int num;
> + struct feature features[0];
> +};
> +
> +#define FPGA_FEATURE_DEV_FME "dfl-fme"
> +#define FPGA_FEATURE_DEV_PORT "dfl-port"
> +
> +static inline int feature_platform_data_size(const int num)
> +{
> + return sizeof(struct feature_platform_data) +
> + num * sizeof(struct feature);
> +}
> +
> +int fpga_port_id(struct platform_device *pdev);
> +
> +static inline int fpga_port_check_id(struct platform_device *pdev,
> + void *pport_id)
> +{
> + return fpga_port_id(pdev) == *(int *)pport_id;
> +}
> +
> +void __fpga_port_enable(struct platform_device *pdev);
> +int __fpga_port_disable(struct platform_device *pdev);
> +
> +static inline void fpga_port_enable(struct platform_device *pdev)
> +{
> + struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +
> + mutex_lock(&pdata->lock);
> + __fpga_port_enable(pdev);
> + mutex_unlock(&pdata->lock);
> +}
> +
> +static inline int fpga_port_disable(struct platform_device *pdev)
> +{
> + struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> + int ret;
> +
> + mutex_lock(&pdata->lock);
> + ret = __fpga_port_disable(pdev);
> + mutex_unlock(&pdata->lock);
> +
> + return ret;
> +}
> +
> +/*
> + * This function resets the FPGA Port and its accelerator (AFU) by function
> + * __fpga_port_disable and __fpga_port_enable (set port soft reset bit and
> + * then clear it). Userspace can do Port reset at any time, e.g during DMA
> + * or Partial Reconfiguration. But it should never cause any system level
> + * issue, only functional failure (e.g DMA or PR operation failure) and be
> + * recoverable from the failure.
> + *
> + * Note: the accelerator (AFU) is not accessible when its port is in reset
> + * (disabled). Any attempts on MMIO access to AFU while in reset, will
> + * result errors reported via port error reporting sub feature (if present).
> + */
> +static inline int __fpga_port_reset(struct platform_device *pdev)
> +{
> + int ret;
> +
> + ret = __fpga_port_disable(pdev);
> + if (ret)
> + return ret;
> +
> + __fpga_port_enable(pdev);
> +
> + return 0;
> +}
> +
> +static inline int fpga_port_reset(struct platform_device *pdev)
> +{
> + struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> + int ret;
> +
> + mutex_lock(&pdata->lock);
> + ret = __fpga_port_reset(pdev);
> + mutex_unlock(&pdata->lock);
> +
> + return ret;
> +}
I'm still scratching my head about how the enumeration code also has
code that handles resetting the PL in a FPGA region and
enabling/disabling the bridge. We've discussed this before [1] and I
know you've looked into it, I'm still trying to figure out how this
can be made modular, so when someone needs to support a different port
in the future, it isn't a complete rewrite.
Speaking of resets, one way forward would be to create a reset
controller for the port (and if possible move the port code to the
bridge platform driver). The current linux-next repo adds support for
reset lookups, so that reset controllers are supported for non-DT
platforms [2].
So the bridge driver would implement the enable/disable functions and
create a reset controller, the fpga-region (or whoever else needs it)
could look the reset controller and use the reset. By using the
kernel reset framework, we don't have to have that piece of code
shared around by having a reset function in a .h file. And it avoids
adding extra dependencies between modules. Also, where necessary, I'd
rather add functionality to the existing bridge/mgr/region frameworks,
adding common interfaces at that level to allow reuse (like adding
status to fpga-mgr). Ideally, this DFL framework would sit on top of
mgr and bridge and allow those to be swapped out for reuse of the DFL
framework on other devices. Also it will save future headaches as mgr
or port implementations evolve.
Alan
[1] https://lkml.org/lkml/2017/12/22/398
[2] https://patchwork.kernel.org/patch/10247475/
> +
> +#define fpga_dev_for_each_feature(pdata, feature) \
> + for ((feature) = (pdata)->features; \
> + (feature) < (pdata)->features + (pdata)->num; (feature)++)
> +
> +static inline struct feature *get_feature_by_id(struct device *dev, u64 id)
> +{
> + struct feature_platform_data *pdata = dev_get_platdata(dev);
> + struct feature *feature;
> +
> + fpga_dev_for_each_feature(pdata, feature)
> + if (feature->id == id)
> + return feature;
> +
> + return NULL;
> +}
> +
> +static inline void __iomem *get_feature_ioaddr_by_id(struct device *dev, u64 id)
> +{
> + struct feature *feature = get_feature_by_id(dev, id);
> +
> + if (feature && feature->ioaddr)
> + return feature->ioaddr;
> +
> + WARN_ON(1);
> + return NULL;
> +}
> +
> +static inline bool feature_is_fme(void __iomem *base)
> +{
> + u64 v = readq(base + DFH);
> +
> + return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> + (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_FME);
> +}
> +
> +static inline bool feature_is_port(void __iomem *base)
> +{
> + u64 v = readq(base + DFH);
> +
> + return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> + (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_PORT);
> +}
> +
> +/**
> + * struct fpga_enum_info - FPGA enumeration information
> + *
> + * @dev: parent device.
> + * @dfls: list of device feature lists.
> + */
> +struct fpga_enum_info {
> + struct device *dev;
> + struct list_head dfls;
> +};
> +
> +/**
> + * struct fpga_enum_dfl - FPGA enumeration device feature list information
> + *
> + * @start: base address of this device feature list.
> + * @len: size of this device feature list.
> + * @ioaddr: mapped base address of this device feature list.
> + * @node: node in list of device feature lists.
> + */
> +struct fpga_enum_dfl {
> + resource_size_t start;
> + resource_size_t len;
> +
> + void __iomem *ioaddr;
> +
> + struct list_head node;
> +};
> +
> +struct fpga_enum_info *fpga_enum_info_alloc(struct device *dev);
> +int fpga_enum_info_add_dfl(struct fpga_enum_info *info, resource_size_t start,
> + resource_size_t len, void __iomem *ioaddr);
> +void fpga_enum_info_free(struct fpga_enum_info *info);
> +
> +/**
> + * struct fpga_cdev - fpga container device
> + *
> + * @parent: parent device of this container device.
> + * @region: base fpga region.
> + * @fme_dev: FME feature device under this container device.
> + * @lock: mutex lock to protect the port device list.
> + * @port_dev_list: list of all port feature devices under this container device.
> + */
> +struct fpga_cdev {
> + struct device *parent;
> +
> + struct fpga_region region;
> +
> + struct device *fme_dev;
> +
> + struct mutex lock; /* to protect the port device list */
> + struct list_head port_dev_list;
> +};
> +
> +struct fpga_cdev *fpga_enumerate_feature_devs(struct fpga_enum_info *info);
> +void fpga_remove_feature_devs(struct fpga_cdev *cdev);
> +
> +#endif /* __FPGA_DFL_H */
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH net-next] modules: allow modprobe load regular elf binaries
From: Luis R. Rodriguez @ 2018-03-22 20:54 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Jessica Yu, Linus Torvalds, Mimi Zohar, Djalal Harouni,
David Miller, Andy Lutomirski, Kees Cook, Alexei Starovoitov,
Al Viro, Daniel Borkmann, Greg Kroah-Hartman, Network Development,
Linux Kernel Mailing List, kernel-team, Linux API,
Luis R. Rodriguez, Michal Hocko, Hannes Reinecke, werner
In-Reply-To: <20180310151652.GV4449@wotan.suse.de>
On Sat, Mar 10, 2018 at 03:16:52PM +0000, Luis R. Rodriguez wrote:
> On Sat, Mar 10, 2018 at 02:08:43PM +0000, Luis R. Rodriguez wrote:
> > The alternative to this would be a simple equivalent of try_then_request_module()
> > for UMH modules: try_umhm_then_request_umh_module() or whatever. So just as I
> > argued earlier over UMH limitations, this is not the end of the world for umh
> > modules, and it doesn't mean you can't get *properly* add umh modules upstream,
> > it would *just mean* we'd be perpetuating today's (IMHO) horrible and loose
> > semantics.
>
> I was about to suggest that perhaps a try_umhm_then_request_umh_module() or
> whatever should not be a macro -- but instead an actual routine, and we don't
> export say the simple form to avoid non-deterministic uses of it from the
> start... but the thing is *it'd have to be a macro* given that the *check* for
> the module *has to be loose*, just as try_then_request_module()...
>
> *Ugh* gross.
>
> Another reason for me to want an actual deterministic clean proper solution
> from the start.
I just thought of another consideration which should be made here for the long
term.
Some init systems have a timeout for kmod workers, that is the userspace
process which issues the modprobe call.
That was very well intentioned, however it ended up being nonsense, so at least
on SLE systemd we disable the timeout for kmod workers. What others do... is
unclear to me. Upstream wise the timeout was increased considerably, however,
*if* such timeout is in effect for users it has some implicit implications on
the number of devices a driver could support:
number_devices = systemd_timeout
-------------------------------------
max known probe time for driver
I've documented the logic to these conclusions [0].
It sounds like we *do* want a full sync wait mechanism, and as I noted I think
we should fix the determinism aspect of it. Since no aliases will be supported
for usermode modules this will be much easier to support, and I can volunteer
to help with that.
However given the above... if we're going to use request_module() API (or a
really fixed deterministic version of it later) for usermode kernel modules,
the limitation above still applies.
Are these usermode modules doing all the handy work on init? Or can it be
deferred once loaded? How much loading on init should a usermode module need?
If we can ensure that these usermode modules don't take *any time at all* on
their init *from the start*, it would be wonderful and we'd end up avoiding
some really odd corner case issues later.
[0] http://www.do-not-panic.com/2015/12/linux-asynchronous-probe.html
Luis
^ permalink raw reply
* Re: [PATCH 06/28] aio: implement IOCB_CMD_POLL
From: Al Viro @ 2018-03-22 18:16 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180322172410.GC5542@lst.de>
On Thu, Mar 22, 2018 at 06:24:10PM +0100, Christoph Hellwig wrote:
> -static void aio_complete(struct aio_kiocb *iocb, long res, long res2)
> +static bool aio_complete(struct aio_kiocb *iocb, long res, long res2,
> + unsigned complete_flags)
Looks like all callers are following that with "if returned true,
fput(something)". Does it really make any sense to keep that struct
file * in different fields?
Wait a sec... What ordering do we want for
* call(s) of ->ki_complete
* call (if any) of ->ki_cancel
* dropping reference to struct file
and what are the expected call chains for all of those?
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 06/28] aio: implement IOCB_CMD_POLL
From: Christoph Hellwig @ 2018-03-22 17:24 UTC (permalink / raw)
To: Al Viro
Cc: Christoph Hellwig, Avi Kivity, linux-aio, linux-fsdevel, netdev,
linux-api, linux-kernel
In-Reply-To: <20180322165255.GI30522@ZenIV.linux.org.uk>
On Thu, Mar 22, 2018 at 04:52:55PM +0000, Al Viro wrote:
> On Wed, Mar 21, 2018 at 08:40:10AM +0100, Christoph Hellwig wrote:
> > Simple one-shot poll through the io_submit() interface. To poll for
> > a file descriptor the application should submit an iocb of type
> > IOCB_CMD_POLL. It will poll the fd for the events specified in the
> > the first 32 bits of the aio_buf field of the iocb.
> >
> > Unlike poll or epoll without EPOLLONESHOT this interface always works
> > in one shot mode, that is once the iocb is completed, it will have to be
> > resubmitted.
>
> AFAICS, your wakeup can race with io_cancel(), leading to double fput().
> You are checking the "somebody had committed itself to cancelling that
> thing" bit outside of ->ctx_lock on the wakeup side, and I don't see
> anything to prevent both getting to __aio_poll_complete() on the same
> iocb, with obvious results.
True. Probably wants something like this to fix, although for this
is entirely untested:
diff --git a/fs/aio.c b/fs/aio.c
index 38b408129697..66d5cc272617 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -187,8 +187,9 @@ struct aio_kiocb {
* for cancellation */
unsigned int flags; /* protected by ctx->ctx_lock */
-#define AIO_IOCB_DELAYED_CANCEL (1 << 0)
-#define AIO_IOCB_CANCELLED (1 << 1)
+#define AIO_IOCB_CAN_CANCEL (1 << 0)
+#define AIO_IOCB_DELAYED_CANCEL (1 << 1)
+#define AIO_IOCB_CANCELLED (1 << 2)
/*
* If the aio_resfd field of the userspace iocb is not zero,
@@ -568,7 +569,7 @@ static void __kiocb_set_cancel_fn(struct aio_kiocb *req,
spin_lock_irqsave(&ctx->ctx_lock, flags);
list_add_tail(&req->ki_list, &ctx->active_reqs);
req->ki_cancel = cancel;
- req->flags |= iocb_flags;
+ req->flags |= (AIO_IOCB_CAN_CANCEL | iocb_flags);
spin_unlock_irqrestore(&ctx->ctx_lock, flags);
}
@@ -1086,22 +1087,30 @@ static struct kioctx *lookup_ioctx(unsigned long ctx_id)
return ret;
}
+#define AIO_COMPLETE_CANCEL (1 << 0)
+
/* aio_complete
* Called when the io request on the given iocb is complete.
*/
-static void aio_complete(struct aio_kiocb *iocb, long res, long res2)
+static bool aio_complete(struct aio_kiocb *iocb, long res, long res2,
+ unsigned complete_flags)
{
struct kioctx *ctx = iocb->ki_ctx;
struct aio_ring *ring;
struct io_event *ev_page, *event;
unsigned tail, pos, head;
- unsigned long flags;
-
- if (!list_empty_careful(iocb->ki_list.next)) {
- unsigned long flags;
+ unsigned long flags;
+ if (iocb->flags & AIO_IOCB_CAN_CANCEL) {
spin_lock_irqsave(&ctx->ctx_lock, flags);
- list_del(&iocb->ki_list);
+ if (!(complete_flags & AIO_COMPLETE_CANCEL) &&
+ (iocb->flags & AIO_IOCB_CANCELLED)) {
+ spin_unlock_irqrestore(&ctx->ctx_lock, flags);
+ return false;
+ }
+
+ if (!list_empty(&iocb->ki_list))
+ list_del(&iocb->ki_list);
spin_unlock_irqrestore(&ctx->ctx_lock, flags);
}
@@ -1177,6 +1186,7 @@ static void aio_complete(struct aio_kiocb *iocb, long res, long res2)
wake_up(&ctx->wait);
percpu_ref_put(&ctx->reqs);
+ return true;
}
/* aio_read_events_ring
@@ -1425,6 +1435,7 @@ SYSCALL_DEFINE1(io_destroy, aio_context_t, ctx)
static void aio_complete_rw(struct kiocb *kiocb, long res, long res2)
{
struct aio_kiocb *iocb = container_of(kiocb, struct aio_kiocb, rw);
+ struct file *file = kiocb->ki_filp;
WARN_ON_ONCE(is_sync_kiocb(kiocb));
@@ -1440,8 +1451,8 @@ static void aio_complete_rw(struct kiocb *kiocb, long res, long res2)
file_end_write(kiocb->ki_filp);
}
- fput(kiocb->ki_filp);
- aio_complete(iocb, res, res2);
+ if (aio_complete(iocb, res, res2, 0))
+ fput(file);
}
static int aio_prep_rw(struct kiocb *req, struct iocb *iocb)
@@ -1584,11 +1595,13 @@ static ssize_t aio_write(struct kiocb *req, struct iocb *iocb, bool vectored,
static void aio_fsync_work(struct work_struct *work)
{
struct fsync_iocb *req = container_of(work, struct fsync_iocb, work);
+ struct aio_kiocb *iocb = container_of(req, struct aio_kiocb, fsync);
+ struct file *file = req->file;
int ret;
ret = vfs_fsync(req->file, req->datasync);
- fput(req->file);
- aio_complete(container_of(req, struct aio_kiocb, fsync), ret, 0);
+ if (aio_complete(iocb, ret, 0, 0))
+ fput(file);
}
static int aio_fsync(struct fsync_iocb *req, struct iocb *iocb, bool datasync)
@@ -1617,27 +1630,23 @@ static int aio_fsync(struct fsync_iocb *req, struct iocb *iocb, bool datasync)
return ret;
}
-static void __aio_complete_poll(struct poll_iocb *req, __poll_t mask)
-{
- fput(req->file);
- aio_complete(container_of(req, struct aio_kiocb, poll),
- mangle_poll(mask), 0);
-}
-
static void aio_complete_poll(struct poll_iocb *req, __poll_t mask)
{
struct aio_kiocb *iocb = container_of(req, struct aio_kiocb, poll);
+ struct file *file = req->file;
- if (!(iocb->flags & AIO_IOCB_CANCELLED))
- __aio_complete_poll(req, mask);
+ if (aio_complete(iocb, mangle_poll(mask), 0, 0))
+ fput(file);
}
static int aio_poll_cancel(struct kiocb *rw)
{
struct aio_kiocb *iocb = container_of(rw, struct aio_kiocb, rw);
+ struct file *file = iocb->poll.file;
remove_wait_queue(iocb->poll.head, &iocb->poll.wait);
- __aio_complete_poll(&iocb->poll, 0); /* no events to report */
+ if (aio_complete(iocb, 0, 0, AIO_COMPLETE_CANCEL))
+ fput(file);
return 0;
}
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply related
* Re: [PATCH 4/9] aio: sanitize ki_list handling
From: Christoph Hellwig @ 2018-03-22 17:04 UTC (permalink / raw)
To: Al Viro
Cc: Christoph Hellwig, Avi Kivity, linux-aio, linux-fsdevel,
linux-api, linux-kernel
In-Reply-To: <20180322152414.GF30522@ZenIV.linux.org.uk>
On Thu, Mar 22, 2018 at 03:24:14PM +0000, Al Viro wrote:
> On Wed, Mar 21, 2018 at 08:32:27AM +0100, Christoph Hellwig wrote:
>
> > - if (iocb->ki_list.next) {
> > + if (!list_empty_careful(iocb->ki_list.next)) {
>
> Umm... Why not list_empty_careful(&iocb->ki_list)?
Yes, that makes a lot more sense.
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 06/28] aio: implement IOCB_CMD_POLL
From: Al Viro @ 2018-03-22 16:52 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180321074032.14211-7-hch@lst.de>
On Wed, Mar 21, 2018 at 08:40:10AM +0100, Christoph Hellwig wrote:
> Simple one-shot poll through the io_submit() interface. To poll for
> a file descriptor the application should submit an iocb of type
> IOCB_CMD_POLL. It will poll the fd for the events specified in the
> the first 32 bits of the aio_buf field of the iocb.
>
> Unlike poll or epoll without EPOLLONESHOT this interface always works
> in one shot mode, that is once the iocb is completed, it will have to be
> resubmitted.
AFAICS, your wakeup can race with io_cancel(), leading to double fput().
You are checking the "somebody had committed itself to cancelling that
thing" bit outside of ->ctx_lock on the wakeup side, and I don't see
anything to prevent both getting to __aio_poll_complete() on the same
iocb, with obvious results.
I might be missing something subtle in there, but then it would be nice to
have it covered in commit message...
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: io_pgetevents & aio fsync
From: Al Viro @ 2018-03-22 16:36 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Avi Kivity, linux-aio, linux-fsdevel, linux-api, linux-kernel
In-Reply-To: <20180322163605.GH30522@ZenIV.linux.org.uk>
On Thu, Mar 22, 2018 at 04:36:05PM +0000, Al Viro wrote:
> On Wed, Mar 21, 2018 at 08:32:23AM +0100, Christoph Hellwig wrote:
> > Hi all,
> >
> > this patch adds workqueue based fsync offload. Version of this
> > patch have been floating around for a couple years, but we now
> > have a user with seastar used by ScyllaDB (who sponsored this
> > work) that really wants this in addition to the aio poll support.
> > More details are in the patch itself.
> >
> > Because the iocb types have been defined sine day one (and probably
> > were supported by RHEL3) libaio already supports these calls as-is.
> >
> > This also pulls in the aio cleanups and io_pgetevents support previously
> > submitted and review as part of the aio poll series. The aio poll
> > series will be resubmitted on top of this series
>
> Everything other than 6/9 looks sane; 6/9 belongs in aio poll series
> and AFAICS its user in there is actually broken.
Gyah... 7/9, that is.
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: io_pgetevents & aio fsync
From: Al Viro @ 2018-03-22 16:36 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Avi Kivity, linux-aio, linux-fsdevel, linux-api, linux-kernel
In-Reply-To: <20180321073232.13366-1-hch@lst.de>
On Wed, Mar 21, 2018 at 08:32:23AM +0100, Christoph Hellwig wrote:
> Hi all,
>
> this patch adds workqueue based fsync offload. Version of this
> patch have been floating around for a couple years, but we now
> have a user with seastar used by ScyllaDB (who sponsored this
> work) that really wants this in addition to the aio poll support.
> More details are in the patch itself.
>
> Because the iocb types have been defined sine day one (and probably
> were supported by RHEL3) libaio already supports these calls as-is.
>
> This also pulls in the aio cleanups and io_pgetevents support previously
> submitted and review as part of the aio poll series. The aio poll
> series will be resubmitted on top of this series
Everything other than 6/9 looks sane; 6/9 belongs in aio poll series
and AFAICS its user in there is actually broken.
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 7/9] aio: add delayed cancel support
From: Al Viro @ 2018-03-22 16:33 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Avi Kivity, linux-aio, linux-fsdevel, linux-api, linux-kernel
In-Reply-To: <20180321073232.13366-8-hch@lst.de>
On Wed, Mar 21, 2018 at 08:32:30AM +0100, Christoph Hellwig wrote:
> The upcoming aio poll support would like to be able to complete the
> iocb inline from the cancellation context, but that would cause
> a lock order reversal. Add support for optionally moving the cancelation
> outside the context lock to avoid this reversal.
Ouch... Seeing that you've just taken out cmpxchg loop out of kiocb_cancel()
with "serialized on ->ctx_lock" for explanation of safety... Let me check
the aio_poll side of it; this commit might be better off in the poll series,
*if* it is actually correct.
What's to prevent double completions there? Suppose we have iocb sitting on
the wait queue; cancellation callback set, so's "delayed cancel" flag.
Now, somebody tries to cancel the fucker on CPU1. With ctx->lock held the
sucker is found on the list and, just as we mark it "cancelled", driver sends
a wakeup, executing (on CPU2) aio_poll_wake(), calling aio_complete_poll()
(without ctx->lock, so no exclusion with io_cancel(2) on CPU1), which checks
AIO_IOCB_CANCELLED and does not notice the flag being set on CPU1, then
proceeds to __aio_complete_poll() and fput() in there.
In the meanwhile, CPU1 has taken the sucker off the list, dropped the
lock and called kiocb_cancel() on it. Now we get aio_poll_cancel()
and __aio_complete_poll() on CPU1, with *another* fput().
What am I missing here that would prevent such a race?
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 4/9] aio: sanitize ki_list handling
From: Al Viro @ 2018-03-22 15:24 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Avi Kivity, linux-aio, linux-fsdevel, linux-api, linux-kernel
In-Reply-To: <20180321073232.13366-5-hch@lst.de>
On Wed, Mar 21, 2018 at 08:32:27AM +0100, Christoph Hellwig wrote:
> - if (iocb->ki_list.next) {
> + if (!list_empty_careful(iocb->ki_list.next)) {
Umm... Why not list_empty_careful(&iocb->ki_list)?
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 28/28] random: convert to ->poll_mask
From: Theodore Y. Ts'o @ 2018-03-22 13:49 UTC (permalink / raw)
To: Christoph Hellwig
Cc: viro, Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180321074032.14211-29-hch@lst.de>
On Wed, Mar 21, 2018 at 08:40:32AM +0100, Christoph Hellwig wrote:
> The big change is that random_read_wait and random_write_wait are merged
> into a single waitqueue that uses keyed wakeups. Because wait_event_*
> doesn't know about that this will lead to occassional spurious wakeups
> in _random_read and add_hwgenerator_randomness, but wait_event_* is
> designed to handle these and were are not in a a hot path there.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Theodore Ts'o <tytso@mit.edu>
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH v4 16/24] fpga: dfl: add fpga manager platform driver for FME
From: Wu Hao @ 2018-03-22 6:07 UTC (permalink / raw)
To: Moritz Fischer
Cc: Alan Tull, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <20180321165552.e6kzvkbpak5hbfwo@us-sjc-wireless-10-16-34-52.amer.corp.natinst.com>
On Wed, Mar 21, 2018 at 09:55:52AM -0700, Moritz Fischer wrote:
> On Wed, Mar 21, 2018 at 10:50:01AM +0800, Wu Hao wrote:
> > On Tue, Mar 20, 2018 at 03:32:34PM -0500, Alan Tull wrote:
> > > On Tue, Feb 13, 2018 at 3:24 AM, Wu Hao <hao.wu@intel.com> wrote:
> > >
> > > Hi Hao,
> > >
> > > Elsewhere we discussed moving #defines used only in this driver either
> > > to this .c file or to a similarly named .h file. A couple minor
> > > things below.
> >
> > Hi Alan,
> >
> > Yes, I will move those #defines into a similarly named .h file.
> >
> > >
> > > > This patch adds fpga manager driver for FPGA Management Engine (FME). It
> > > > implements fpga_manager_ops for FPGA Partial Reconfiguration function.
> > > >
> > > > Signed-off-by: Tim Whisonant <tim.whisonant@intel.com>
> > > > Signed-off-by: Enno Luebbers <enno.luebbers@intel.com>
> > > > Signed-off-by: Shiva Rao <shiva.rao@intel.com>
> > > > Signed-off-by: Christopher Rauer <christopher.rauer@intel.com>
> > > > Signed-off-by: Kang Luwei <luwei.kang@intel.com>
> > > > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> > > > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > > > ---
> > > > v3: rename driver to dfl-fpga-fme-mgr
> > > > implemented status callback for fpga manager
> > > > rebased due to fpga api changes
> > > > v4: rename to dfl-fme-mgr, and fix SPDX license issue
> > > > add pr_credit comments and improve dev_err message
> > > > remove interface_id sysfs interface
> > > > include dfl-fme-pr.h instead of dfl.h
> > > > ---
> > > > drivers/fpga/Kconfig | 6 +
> > > > drivers/fpga/Makefile | 1 +
> > > > drivers/fpga/dfl-fme-mgr.c | 290 +++++++++++++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 297 insertions(+)
> > > > create mode 100644 drivers/fpga/dfl-fme-mgr.c
> > > >
> > > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > > > index 103d5e2..89f76e8 100644
> > > > --- a/drivers/fpga/Kconfig
> > > > +++ b/drivers/fpga/Kconfig
> > > > @@ -150,6 +150,12 @@ config FPGA_DFL_FME
> > > > FPGA platform level management features. There shall be 1 FME
> > > > per DFL based FPGA device.
> > > >
> > > > +config FPGA_DFL_FME_MGR
> > > > + tristate "FPGA DFL FME Manager Driver"
> > > > + depends on FPGA_DFL_FME
> > > > + help
> > > > + Say Y to enable FPGA Manager driver for FPGA Management Engine.
> > > > +
> > > > config FPGA_DFL_PCI
> > > > tristate "FPGA Device Feature List (DFL) PCIe Device Driver"
> > > > depends on PCI && FPGA_DFL
> > > > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > > > index 3c44fc9..f82814a 100644
> > > > --- a/drivers/fpga/Makefile
> > > > +++ b/drivers/fpga/Makefile
> > > > @@ -31,6 +31,7 @@ obj-$(CONFIG_OF_FPGA_REGION) += of-fpga-region.o
> > > > # FPGA Device Feature List Support
> > > > obj-$(CONFIG_FPGA_DFL) += dfl.o
> > > > obj-$(CONFIG_FPGA_DFL_FME) += dfl-fme.o
> > > > +obj-$(CONFIG_FPGA_DFL_FME_MGR) += dfl-fme-mgr.o
> > > >
> > > > dfl-fme-objs := dfl-fme-main.o dfl-fme-pr.o
> > > >
> > > > diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
> > > > new file mode 100644
> > > > index 0000000..2f92c29
> > > > --- /dev/null
> > > > +++ b/drivers/fpga/dfl-fme-mgr.c
> > > > @@ -0,0 +1,290 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * FPGA Manager Driver for FPGA Management Engine (FME)
> > > > + *
> > > > + * Copyright (C) 2017 Intel Corporation, Inc.
> > > > + *
> > > > + * Authors:
> > > > + * Kang Luwei <luwei.kang@intel.com>
> > > > + * Xiao Guangrong <guangrong.xiao@linux.intel.com>
> > > > + * Wu Hao <hao.wu@intel.com>
> > > > + * Joseph Grecco <joe.grecco@intel.com>
> > > > + * Enno Luebbers <enno.luebbers@intel.com>
> > > > + * Tim Whisonant <tim.whisonant@intel.com>
> > > > + * Ananda Ravuri <ananda.ravuri@intel.com>
> > > > + * Christopher Rauer <christopher.rauer@intel.com>
> > > > + * Henry Mitchel <henry.mitchel@intel.com>
> > > > + */
> > > > +
> > > > +#include <linux/bitfield.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/iopoll.h>
> > > > +#include <linux/fpga/fpga-mgr.h>
> > > > +
> > > > +#include "dfl-fme-pr.h"
> > > > +
> > > > +#define PR_WAIT_TIMEOUT 8000000
> > > > +#define PR_HOST_STATUS_IDLE 0
> > > > +
> > > > +struct fme_mgr_priv {
> > > > + void __iomem *ioaddr;
> > > > + u64 pr_error;
> > > > +};
> > > > +
> > > > +static u64 pr_error_to_mgr_status(u64 err)
> > > > +{
> > > > + u64 status = 0;
> > > > +
> > > > + if (err & FME_PR_ERR_OPERATION_ERR)
> > > > + status |= FPGA_MGR_STATUS_OPERATION_ERR;
> > > > + if (err & FME_PR_ERR_CRC_ERR)
> > > > + status |= FPGA_MGR_STATUS_CRC_ERR;
> > > > + if (err & FME_PR_ERR_INCOMPATIBLE_BS)
> > > > + status |= FPGA_MGR_STATUS_INCOMPATIBLE_IMAGE_ERR;
> > > > + if (err & FME_PR_ERR_PROTOCOL_ERR)
> > > > + status |= FPGA_MGR_STATUS_IP_PROTOCOL_ERR;
> > > > + if (err & FME_PR_ERR_FIFO_OVERFLOW)
> > > > + status |= FPGA_MGR_STATUS_FIFO_OVERFLOW_ERR;
> > > > +
> > > > + return status;
> > > > +}
> > > > +
> > > > +static u64 fme_mgr_pr_error_handle(void __iomem *fme_pr)
> > > > +{
> > > > + u64 pr_status, pr_error;
> > > > +
> > > > + pr_status = readq(fme_pr + FME_PR_STS);
> > > > + if (!(pr_status & FME_PR_STS_PR_STS))
> > > > + return 0;
> > > > +
> > > > + pr_error = readq(fme_pr + FME_PR_ERR);
> > > > + writeq(pr_error, fme_pr + FME_PR_ERR);
> > > > +
> > > > + return pr_error;
> > > > +}
> > > > +
> > > > +static int fme_mgr_write_init(struct fpga_manager *mgr,
> > > > + struct fpga_image_info *info,
> > > > + const char *buf, size_t count)
> > > > +{
> > > > + struct device *dev = &mgr->dev;
> > > > + struct fme_mgr_priv *priv = mgr->priv;
> > > > + void __iomem *fme_pr = priv->ioaddr;
> > > > + u64 pr_ctrl, pr_status;
> > > > +
> > > > + if (!(info->flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> > > > + dev_err(dev, "only supports partial reconfiguration.\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + dev_dbg(dev, "resetting PR before initiated PR\n");
> > > > +
> > > > + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
> > > > + pr_ctrl |= FME_PR_CTRL_PR_RST;
> > > > + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
> > > > +
> > > > + if (readq_poll_timeout(fme_pr + FME_PR_CTRL, pr_ctrl,
> > > > + pr_ctrl & FME_PR_CTRL_PR_RSTACK, 1,
> > > > + PR_WAIT_TIMEOUT)) {
> > > > + dev_err(dev, "PR Reset ACK timeout\n");
> > > > + return -ETIMEDOUT;
> > > > + }
> > > > +
> > > > + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
> > > > + pr_ctrl &= ~FME_PR_CTRL_PR_RST;
> > > > + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
> > > > +
> > > > + dev_dbg(dev,
> > > > + "waiting for PR resource in HW to be initialized and ready\n");
> > > > +
> > > > + if (readq_poll_timeout(fme_pr + FME_PR_STS, pr_status,
> > > > + (pr_status & FME_PR_STS_PR_STS) ==
> > > > + FME_PR_STS_PR_STS_IDLE, 1, PR_WAIT_TIMEOUT)) {
> > > > + dev_err(dev, "PR Status timeout\n");
> > > > + priv->pr_error = fme_mgr_pr_error_handle(fme_pr);
> > > > + return -ETIMEDOUT;
> > > > + }
> > > > +
> > > > + dev_dbg(dev, "check and clear previous PR error\n");
> > > > + priv->pr_error = fme_mgr_pr_error_handle(fme_pr);
> > > > + if (priv->pr_error)
> > > > + dev_dbg(dev, "previous PR error detected %llx\n",
> > > > + (unsigned long long)priv->pr_error);
> > > > +
> > > > + dev_dbg(dev, "set PR port ID\n");
> > > > +
> > > > + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
> > > > + pr_ctrl &= ~FME_PR_CTRL_PR_RGN_ID;
> > > > + pr_ctrl |= FIELD_PREP(FME_PR_CTRL_PR_RGN_ID, info->region_id);
> > > > + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int fme_mgr_write(struct fpga_manager *mgr,
> > > > + const char *buf, size_t count)
> > > > +{
> > > > + struct device *dev = &mgr->dev;
> > > > + struct fme_mgr_priv *priv = mgr->priv;
> > > > + void __iomem *fme_pr = priv->ioaddr;
> > > > + u64 pr_ctrl, pr_status, pr_data;
> > > > + int delay = 0, pr_credit, i = 0;
> > > > +
> > > > + dev_dbg(dev, "start request\n");
> > > > +
> > > > + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
> > > > + pr_ctrl |= FME_PR_CTRL_PR_START;
> > > > + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
> > > > +
> > > > + dev_dbg(dev, "pushing data from bitstream to HW\n");
> > > > +
> > > > + /*
> > > > + * driver can push data to PR hardware using PR_DATA register once HW
> > > > + * has enough pr_credit (> 1), pr_credit reduces one for every 32bit
> > > > + * pr data write to PR_DATA register. If pr_credit <= 1, driver needs
> > > > + * to wait for enough pr_credit from hardware by polling.
> > > > + */
> > > > + pr_status = readq(fme_pr + FME_PR_STS);
> > > > + pr_credit = FIELD_GET(FME_PR_STS_PR_CREDIT, pr_status);
> > > > +
> > > > + while (count > 0) {
> > > > + while (pr_credit <= 1) {
> > > > + if (delay++ > PR_WAIT_TIMEOUT) {
> > > > + dev_err(dev, "PR_CREDIT timeout\n");
> > > > + return -ETIMEDOUT;
> > > > + }
> > > > + udelay(1);
> > > > +
> > > > + pr_status = readq(fme_pr + FME_PR_STS);
> > > > + pr_credit = FIELD_GET(FME_PR_STS_PR_CREDIT, pr_status);
> > > > + }
> > > > +
> > > > + if (count >= 4) {
> > > > + pr_data = 0;
> > > > + pr_data |= FIELD_PREP(FME_PR_DATA_PR_DATA_RAW,
> > > > + *(((u32 *)buf) + i));
> > > > + writeq(pr_data, fme_pr + FME_PR_DATA);
> > > > + count -= 4;
> > > > + pr_credit--;
> > > > + i++;
> > > > + } else {
> > > > + WARN_ON(1);
> > > > + return -EINVAL;
> > > > + }
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int fme_mgr_write_complete(struct fpga_manager *mgr,
> > > > + struct fpga_image_info *info)
> > > > +{
> > > > + struct device *dev = &mgr->dev;
> > > > + struct fme_mgr_priv *priv = mgr->priv;
> > > > + void __iomem *fme_pr = priv->ioaddr;
> > > > + u64 pr_ctrl;
> > > > +
> > > > + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
> > > > + pr_ctrl |= FME_PR_CTRL_PR_COMPLETE;
> > > > + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
> > > > +
> > > > + dev_dbg(dev, "green bitstream push complete\n");
> > > > + dev_dbg(dev, "waiting for HW to release PR resource\n");
> > > > +
> > > > + if (readq_poll_timeout(fme_pr + FME_PR_CTRL, pr_ctrl,
> > > > + !(pr_ctrl & FME_PR_CTRL_PR_START), 1,
> > > > + PR_WAIT_TIMEOUT)) {
> > > > + dev_err(dev, "PR Completion ACK timeout.\n");
> > > > + return -ETIMEDOUT;
> > > > + }
> > > > +
> > > > + dev_dbg(dev, "PR operation complete, checking status\n");
> > > > + priv->pr_error = fme_mgr_pr_error_handle(fme_pr);
> > > > + if (priv->pr_error) {
> > > > + dev_dbg(dev, "PR error detected %llx\n",
> > > > + (unsigned long long)priv->pr_error);
> > > > + return -EIO;
> > > > + }
> > > > +
> > > > + dev_dbg(dev, "PR done successfully\n");
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static enum fpga_mgr_states fme_mgr_state(struct fpga_manager *mgr)
> > > > +{
> > > > + return FPGA_MGR_STATE_UNKNOWN;
> > > > +}
> > > > +
> > > > +static u64 fme_mgr_status(struct fpga_manager *mgr)
> > > > +{
> > > > + struct fme_mgr_priv *priv = mgr->priv;
> > > > +
> > > > + return pr_error_to_mgr_status(priv->pr_error);
> > > > +}
> > > > +
> > > > +static const struct fpga_manager_ops fme_mgr_ops = {
> > > > + .write_init = fme_mgr_write_init,
> > > > + .write = fme_mgr_write,
> > > > + .write_complete = fme_mgr_write_complete,
> > > > + .state = fme_mgr_state,
> > > > + .status = fme_mgr_status,
> > > > +};
> > > > +
> > > > +static int fme_mgr_probe(struct platform_device *pdev)
> > > > +{
> > > > + struct device *dev = &pdev->dev;
> > > > + struct fme_mgr_priv *priv;
> > > > + struct fpga_manager *mgr;
> > > > + struct resource *res;
> > > > + int ret;
> > > > +
> > > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > > + if (!priv)
> > > > + return -ENOMEM;
> > > > +
> > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > + priv->ioaddr = devm_ioremap(dev, res->start, resource_size(res));
> > >
> > > How about using devm_ioremap_resourc(dev, res) here instead?
> >
> > Actually the register region has already been mapped in lower level driver
> > (e.g pci) so I think we don't have to map the second time here. I plan to
> > add some code to pass the ioaddr via the platform data, and check if valid
> > ioaddr from the platform data firstly in this probe function. If no pdata
> > or no valid ioaddr, then go with devm_ioremap_resource. :)
>
> If you end up sharing register spaces between drivers is regmap / syscon
> maybe a good idea?
Hi Moritz,
Thanks for the comments. I tried the regmap_mmio before but it failed to use
regmap_read and regmap_write for register access. It requires to use writeq /
readq for 64bit registers on the hardware, but regmap_read and regmap_write
only accepts unsigned int value as input parameter. So I only replaced the
bitfields struct/union with macro.
Thanks
Hao
> >
> > >
> > > > + if (IS_ERR(priv->ioaddr))
> > > > + return PTR_ERR(priv->ioaddr);
> > > > +
> > > > + mgr = devm_kzalloc(dev, sizeof(*mgr), GFP_KERNEL);
> > > > + if (!mgr)
> > > > + return -ENOMEM;
> > > > +
> > > > + mgr->name = "DFL FPGA Manager";
> > > > + mgr->mops = &fme_mgr_ops;
> > > > + mgr->priv = priv;
> > > > + mgr->parent = dev;
> > > > + platform_set_drvdata(pdev, mgr);
> > > > +
> > > > + ret = fpga_mgr_register(mgr);
> > > > + if (ret)
> > > > + dev_err(dev, "unable to register FPGA manager\n");
> > > > +
> > > > + return ret;
> > >
> > > You can probably just do "return fpga_mgr_register(mgr);" here.
> >
> > Yes, it looks better, I will fix it. Thanks a lot for the review.
> >
> > Hao
> >
> > >
> > > Thanks,
> > > Alan
> > >
> > > > +}
> > > > +
> > > > +static int fme_mgr_remove(struct platform_device *pdev)
> > > > +{
> > > > + struct fpga_manager *mgr = platform_get_drvdata(pdev);
> > > > +
> > > > + fpga_mgr_unregister(mgr);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static struct platform_driver fme_mgr_driver = {
> > > > + .driver = {
> > > > + .name = FPGA_DFL_FME_MGR,
> > > > + },
> > > > + .probe = fme_mgr_probe,
> > > > + .remove = fme_mgr_remove,
> > > > +};
> > > > +
> > > > +module_platform_driver(fme_mgr_driver);
> > > > +
> > > > +MODULE_DESCRIPTION("FPGA Manager for DFL FPGA Management Engine");
> > > > +MODULE_AUTHOR("Intel Corporation");
> > > > +MODULE_LICENSE("GPL v2");
> > > > +MODULE_ALIAS("platform:dfl-fme-mgr");
> > > > --
> > > > 2.7.4
> > > >
>
> Cheers,
> Moritz
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 23/24] fpga: dfl: afu: add user afu sub feature support
From: Wu Hao @ 2018-03-22 4:41 UTC (permalink / raw)
To: Alan Tull
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Xiao Guangrong, Tim Whisonant, Enno Luebbers,
Shiva Rao, Christopher Rauer
In-Reply-To: <CANk1AXSyh1ZcbgVbj3DxjULEeYKMfq_4CA6P0DK6+_z-pnBMUw@mail.gmail.com>
On Wed, Mar 21, 2018 at 06:50:54PM -0500, Alan Tull wrote:
> On Tue, Mar 20, 2018 at 2:10 AM, Wu Hao <hao.wu@intel.com> wrote:
>
> >> > +static int afu_mmap(struct file *filp, struct vm_area_struct *vma)
> >> > +{
> >> > + struct fpga_afu_region region;
> >> > + struct platform_device *pdev = filp->private_data;
> >> > + struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> >> > + u64 size = vma->vm_end - vma->vm_start;
> >> > + u64 offset;
> >> > + int ret;
> >> > +
> >> > + if (!(vma->vm_flags & VM_SHARED))
> >> > + return -EINVAL;
> >> > +
> >> > + offset = vma->vm_pgoff << PAGE_SHIFT;
> >> > + ret = afu_get_region_by_offset(pdata, offset, size, ®ion);
>
> Most of the functions here are afu_region_*, but there's also
> afu_get_region_by_*. Better if afu_region_get_by_* to be consistent.
Agree, will fix it in the next version, thanks a lot for the comments.
Hao
^ permalink raw reply
* Re: [PATCH v4 04/24] fpga: add device feature list support
From: Wu Hao @ 2018-03-22 4:40 UTC (permalink / raw)
To: Alan Tull
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <CANk1AXT0dBT9qejdbSyi75e956OHA0-y0V7de7H=XkOZzVhRqQ@mail.gmail.com>
On Wed, Mar 21, 2018 at 06:54:58PM -0500, Alan Tull wrote:
> On Tue, Feb 13, 2018 at 3:24 AM, Wu Hao <hao.wu@intel.com> wrote:
>
> Hi Hao,
>
> > +static int
> > +build_info_create_dev(struct build_feature_devs_info *binfo,
> > + enum fpga_id_type type, const char *name,
> > + void __iomem *ioaddr)
> > +{
> > + struct platform_device *fdev;
> > + int ret;
> > +
> > + /* we will create a new device, commit current device first */
> > + ret = build_info_commit_dev(binfo);
> > + if (ret)
> > + return ret;
> > +
> > + /*
> > + * we use -ENODEV as the initialization indicator which indicates
> > + * whether the id need to be reclaimed
> > + */
> > + fdev = platform_device_alloc(name, -ENODEV);
> > + if (!fdev)
> > + return -ENOMEM;
> > +
> > + binfo->feature_dev = fdev;
> > + binfo->feature_num = 0;
> > + binfo->ioaddr = ioaddr;
> > + INIT_LIST_HEAD(&binfo->sub_features);
> > +
> > + fdev->id = alloc_fpga_id(type, &fdev->dev);
> > + if (fdev->id < 0)
> > + return fdev->id;
>
> Do we need platform_device_put if returning here?
Hi Alan,
Actually if any error returned by this build_info_create_dev, it will cause
parse_feature_list returns error code and finally build_info_free is invoked.
In build_info_free function, it will do platform_device_put there, so we don't
have to add code to do put here. : )
Thanks
Hao
>
> Alan
>
> > +
> > + fdev->dev.parent = &binfo->cdev->region.dev;
> > +
> > + return 0;
> > +}
> > +
^ permalink raw reply
* Re: [PATCH v4 04/24] fpga: add device feature list support
From: Alan Tull @ 2018-03-21 23:54 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <1518513893-4719-5-git-send-email-hao.wu@intel.com>
On Tue, Feb 13, 2018 at 3:24 AM, Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
> +static int
> +build_info_create_dev(struct build_feature_devs_info *binfo,
> + enum fpga_id_type type, const char *name,
> + void __iomem *ioaddr)
> +{
> + struct platform_device *fdev;
> + int ret;
> +
> + /* we will create a new device, commit current device first */
> + ret = build_info_commit_dev(binfo);
> + if (ret)
> + return ret;
> +
> + /*
> + * we use -ENODEV as the initialization indicator which indicates
> + * whether the id need to be reclaimed
> + */
> + fdev = platform_device_alloc(name, -ENODEV);
> + if (!fdev)
> + return -ENOMEM;
> +
> + binfo->feature_dev = fdev;
> + binfo->feature_num = 0;
> + binfo->ioaddr = ioaddr;
> + INIT_LIST_HEAD(&binfo->sub_features);
> +
> + fdev->id = alloc_fpga_id(type, &fdev->dev);
> + if (fdev->id < 0)
> + return fdev->id;
Do we need platform_device_put if returning here?
Alan
> +
> + fdev->dev.parent = &binfo->cdev->region.dev;
> +
> + return 0;
> +}
> +
^ permalink raw reply
* Re: [PATCH v4 23/24] fpga: dfl: afu: add user afu sub feature support
From: Alan Tull @ 2018-03-21 23:50 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Xiao Guangrong, Tim Whisonant, Enno Luebbers,
Shiva Rao, Christopher Rauer
In-Reply-To: <20180320071037.GB31239@hao-dev>
On Tue, Mar 20, 2018 at 2:10 AM, Wu Hao <hao.wu@intel.com> wrote:
>> > +static int afu_mmap(struct file *filp, struct vm_area_struct *vma)
>> > +{
>> > + struct fpga_afu_region region;
>> > + struct platform_device *pdev = filp->private_data;
>> > + struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
>> > + u64 size = vma->vm_end - vma->vm_start;
>> > + u64 offset;
>> > + int ret;
>> > +
>> > + if (!(vma->vm_flags & VM_SHARED))
>> > + return -EINVAL;
>> > +
>> > + offset = vma->vm_pgoff << PAGE_SHIFT;
>> > + ret = afu_get_region_by_offset(pdata, offset, size, ®ion);
Most of the functions here are afu_region_*, but there's also
afu_get_region_by_*. Better if afu_region_get_by_* to be consistent.
>> > + if (ret)
>> > + return ret;
>> > +
>> > + if (!(region.flags & FPGA_REGION_MMAP))
>>
>> FPGA_REGION_*? We already have FPGA regions which are something
>> different. Please call this something else.
>
> Yes, will replace it with DFL_FPGA_*.
^ permalink raw reply
* Re: [PATCH v4 16/24] fpga: dfl: add fpga manager platform driver for FME
From: Moritz Fischer @ 2018-03-21 16:55 UTC (permalink / raw)
To: Wu Hao
Cc: Alan Tull, Moritz Fischer, linux-fpga, linux-kernel, linux-api,
Kang, Luwei, Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <20180321025001.GA3489@hao-dev>
On Wed, Mar 21, 2018 at 10:50:01AM +0800, Wu Hao wrote:
> On Tue, Mar 20, 2018 at 03:32:34PM -0500, Alan Tull wrote:
> > On Tue, Feb 13, 2018 at 3:24 AM, Wu Hao <hao.wu@intel.com> wrote:
> >
> > Hi Hao,
> >
> > Elsewhere we discussed moving #defines used only in this driver either
> > to this .c file or to a similarly named .h file. A couple minor
> > things below.
>
> Hi Alan,
>
> Yes, I will move those #defines into a similarly named .h file.
>
> >
> > > This patch adds fpga manager driver for FPGA Management Engine (FME). It
> > > implements fpga_manager_ops for FPGA Partial Reconfiguration function.
> > >
> > > Signed-off-by: Tim Whisonant <tim.whisonant@intel.com>
> > > Signed-off-by: Enno Luebbers <enno.luebbers@intel.com>
> > > Signed-off-by: Shiva Rao <shiva.rao@intel.com>
> > > Signed-off-by: Christopher Rauer <christopher.rauer@intel.com>
> > > Signed-off-by: Kang Luwei <luwei.kang@intel.com>
> > > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> > > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > > ---
> > > v3: rename driver to dfl-fpga-fme-mgr
> > > implemented status callback for fpga manager
> > > rebased due to fpga api changes
> > > v4: rename to dfl-fme-mgr, and fix SPDX license issue
> > > add pr_credit comments and improve dev_err message
> > > remove interface_id sysfs interface
> > > include dfl-fme-pr.h instead of dfl.h
> > > ---
> > > drivers/fpga/Kconfig | 6 +
> > > drivers/fpga/Makefile | 1 +
> > > drivers/fpga/dfl-fme-mgr.c | 290 +++++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 297 insertions(+)
> > > create mode 100644 drivers/fpga/dfl-fme-mgr.c
> > >
> > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > > index 103d5e2..89f76e8 100644
> > > --- a/drivers/fpga/Kconfig
> > > +++ b/drivers/fpga/Kconfig
> > > @@ -150,6 +150,12 @@ config FPGA_DFL_FME
> > > FPGA platform level management features. There shall be 1 FME
> > > per DFL based FPGA device.
> > >
> > > +config FPGA_DFL_FME_MGR
> > > + tristate "FPGA DFL FME Manager Driver"
> > > + depends on FPGA_DFL_FME
> > > + help
> > > + Say Y to enable FPGA Manager driver for FPGA Management Engine.
> > > +
> > > config FPGA_DFL_PCI
> > > tristate "FPGA Device Feature List (DFL) PCIe Device Driver"
> > > depends on PCI && FPGA_DFL
> > > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > > index 3c44fc9..f82814a 100644
> > > --- a/drivers/fpga/Makefile
> > > +++ b/drivers/fpga/Makefile
> > > @@ -31,6 +31,7 @@ obj-$(CONFIG_OF_FPGA_REGION) += of-fpga-region.o
> > > # FPGA Device Feature List Support
> > > obj-$(CONFIG_FPGA_DFL) += dfl.o
> > > obj-$(CONFIG_FPGA_DFL_FME) += dfl-fme.o
> > > +obj-$(CONFIG_FPGA_DFL_FME_MGR) += dfl-fme-mgr.o
> > >
> > > dfl-fme-objs := dfl-fme-main.o dfl-fme-pr.o
> > >
> > > diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
> > > new file mode 100644
> > > index 0000000..2f92c29
> > > --- /dev/null
> > > +++ b/drivers/fpga/dfl-fme-mgr.c
> > > @@ -0,0 +1,290 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * FPGA Manager Driver for FPGA Management Engine (FME)
> > > + *
> > > + * Copyright (C) 2017 Intel Corporation, Inc.
> > > + *
> > > + * Authors:
> > > + * Kang Luwei <luwei.kang@intel.com>
> > > + * Xiao Guangrong <guangrong.xiao@linux.intel.com>
> > > + * Wu Hao <hao.wu@intel.com>
> > > + * Joseph Grecco <joe.grecco@intel.com>
> > > + * Enno Luebbers <enno.luebbers@intel.com>
> > > + * Tim Whisonant <tim.whisonant@intel.com>
> > > + * Ananda Ravuri <ananda.ravuri@intel.com>
> > > + * Christopher Rauer <christopher.rauer@intel.com>
> > > + * Henry Mitchel <henry.mitchel@intel.com>
> > > + */
> > > +
> > > +#include <linux/bitfield.h>
> > > +#include <linux/module.h>
> > > +#include <linux/iopoll.h>
> > > +#include <linux/fpga/fpga-mgr.h>
> > > +
> > > +#include "dfl-fme-pr.h"
> > > +
> > > +#define PR_WAIT_TIMEOUT 8000000
> > > +#define PR_HOST_STATUS_IDLE 0
> > > +
> > > +struct fme_mgr_priv {
> > > + void __iomem *ioaddr;
> > > + u64 pr_error;
> > > +};
> > > +
> > > +static u64 pr_error_to_mgr_status(u64 err)
> > > +{
> > > + u64 status = 0;
> > > +
> > > + if (err & FME_PR_ERR_OPERATION_ERR)
> > > + status |= FPGA_MGR_STATUS_OPERATION_ERR;
> > > + if (err & FME_PR_ERR_CRC_ERR)
> > > + status |= FPGA_MGR_STATUS_CRC_ERR;
> > > + if (err & FME_PR_ERR_INCOMPATIBLE_BS)
> > > + status |= FPGA_MGR_STATUS_INCOMPATIBLE_IMAGE_ERR;
> > > + if (err & FME_PR_ERR_PROTOCOL_ERR)
> > > + status |= FPGA_MGR_STATUS_IP_PROTOCOL_ERR;
> > > + if (err & FME_PR_ERR_FIFO_OVERFLOW)
> > > + status |= FPGA_MGR_STATUS_FIFO_OVERFLOW_ERR;
> > > +
> > > + return status;
> > > +}
> > > +
> > > +static u64 fme_mgr_pr_error_handle(void __iomem *fme_pr)
> > > +{
> > > + u64 pr_status, pr_error;
> > > +
> > > + pr_status = readq(fme_pr + FME_PR_STS);
> > > + if (!(pr_status & FME_PR_STS_PR_STS))
> > > + return 0;
> > > +
> > > + pr_error = readq(fme_pr + FME_PR_ERR);
> > > + writeq(pr_error, fme_pr + FME_PR_ERR);
> > > +
> > > + return pr_error;
> > > +}
> > > +
> > > +static int fme_mgr_write_init(struct fpga_manager *mgr,
> > > + struct fpga_image_info *info,
> > > + const char *buf, size_t count)
> > > +{
> > > + struct device *dev = &mgr->dev;
> > > + struct fme_mgr_priv *priv = mgr->priv;
> > > + void __iomem *fme_pr = priv->ioaddr;
> > > + u64 pr_ctrl, pr_status;
> > > +
> > > + if (!(info->flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> > > + dev_err(dev, "only supports partial reconfiguration.\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + dev_dbg(dev, "resetting PR before initiated PR\n");
> > > +
> > > + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
> > > + pr_ctrl |= FME_PR_CTRL_PR_RST;
> > > + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
> > > +
> > > + if (readq_poll_timeout(fme_pr + FME_PR_CTRL, pr_ctrl,
> > > + pr_ctrl & FME_PR_CTRL_PR_RSTACK, 1,
> > > + PR_WAIT_TIMEOUT)) {
> > > + dev_err(dev, "PR Reset ACK timeout\n");
> > > + return -ETIMEDOUT;
> > > + }
> > > +
> > > + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
> > > + pr_ctrl &= ~FME_PR_CTRL_PR_RST;
> > > + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
> > > +
> > > + dev_dbg(dev,
> > > + "waiting for PR resource in HW to be initialized and ready\n");
> > > +
> > > + if (readq_poll_timeout(fme_pr + FME_PR_STS, pr_status,
> > > + (pr_status & FME_PR_STS_PR_STS) ==
> > > + FME_PR_STS_PR_STS_IDLE, 1, PR_WAIT_TIMEOUT)) {
> > > + dev_err(dev, "PR Status timeout\n");
> > > + priv->pr_error = fme_mgr_pr_error_handle(fme_pr);
> > > + return -ETIMEDOUT;
> > > + }
> > > +
> > > + dev_dbg(dev, "check and clear previous PR error\n");
> > > + priv->pr_error = fme_mgr_pr_error_handle(fme_pr);
> > > + if (priv->pr_error)
> > > + dev_dbg(dev, "previous PR error detected %llx\n",
> > > + (unsigned long long)priv->pr_error);
> > > +
> > > + dev_dbg(dev, "set PR port ID\n");
> > > +
> > > + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
> > > + pr_ctrl &= ~FME_PR_CTRL_PR_RGN_ID;
> > > + pr_ctrl |= FIELD_PREP(FME_PR_CTRL_PR_RGN_ID, info->region_id);
> > > + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int fme_mgr_write(struct fpga_manager *mgr,
> > > + const char *buf, size_t count)
> > > +{
> > > + struct device *dev = &mgr->dev;
> > > + struct fme_mgr_priv *priv = mgr->priv;
> > > + void __iomem *fme_pr = priv->ioaddr;
> > > + u64 pr_ctrl, pr_status, pr_data;
> > > + int delay = 0, pr_credit, i = 0;
> > > +
> > > + dev_dbg(dev, "start request\n");
> > > +
> > > + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
> > > + pr_ctrl |= FME_PR_CTRL_PR_START;
> > > + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
> > > +
> > > + dev_dbg(dev, "pushing data from bitstream to HW\n");
> > > +
> > > + /*
> > > + * driver can push data to PR hardware using PR_DATA register once HW
> > > + * has enough pr_credit (> 1), pr_credit reduces one for every 32bit
> > > + * pr data write to PR_DATA register. If pr_credit <= 1, driver needs
> > > + * to wait for enough pr_credit from hardware by polling.
> > > + */
> > > + pr_status = readq(fme_pr + FME_PR_STS);
> > > + pr_credit = FIELD_GET(FME_PR_STS_PR_CREDIT, pr_status);
> > > +
> > > + while (count > 0) {
> > > + while (pr_credit <= 1) {
> > > + if (delay++ > PR_WAIT_TIMEOUT) {
> > > + dev_err(dev, "PR_CREDIT timeout\n");
> > > + return -ETIMEDOUT;
> > > + }
> > > + udelay(1);
> > > +
> > > + pr_status = readq(fme_pr + FME_PR_STS);
> > > + pr_credit = FIELD_GET(FME_PR_STS_PR_CREDIT, pr_status);
> > > + }
> > > +
> > > + if (count >= 4) {
> > > + pr_data = 0;
> > > + pr_data |= FIELD_PREP(FME_PR_DATA_PR_DATA_RAW,
> > > + *(((u32 *)buf) + i));
> > > + writeq(pr_data, fme_pr + FME_PR_DATA);
> > > + count -= 4;
> > > + pr_credit--;
> > > + i++;
> > > + } else {
> > > + WARN_ON(1);
> > > + return -EINVAL;
> > > + }
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int fme_mgr_write_complete(struct fpga_manager *mgr,
> > > + struct fpga_image_info *info)
> > > +{
> > > + struct device *dev = &mgr->dev;
> > > + struct fme_mgr_priv *priv = mgr->priv;
> > > + void __iomem *fme_pr = priv->ioaddr;
> > > + u64 pr_ctrl;
> > > +
> > > + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
> > > + pr_ctrl |= FME_PR_CTRL_PR_COMPLETE;
> > > + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
> > > +
> > > + dev_dbg(dev, "green bitstream push complete\n");
> > > + dev_dbg(dev, "waiting for HW to release PR resource\n");
> > > +
> > > + if (readq_poll_timeout(fme_pr + FME_PR_CTRL, pr_ctrl,
> > > + !(pr_ctrl & FME_PR_CTRL_PR_START), 1,
> > > + PR_WAIT_TIMEOUT)) {
> > > + dev_err(dev, "PR Completion ACK timeout.\n");
> > > + return -ETIMEDOUT;
> > > + }
> > > +
> > > + dev_dbg(dev, "PR operation complete, checking status\n");
> > > + priv->pr_error = fme_mgr_pr_error_handle(fme_pr);
> > > + if (priv->pr_error) {
> > > + dev_dbg(dev, "PR error detected %llx\n",
> > > + (unsigned long long)priv->pr_error);
> > > + return -EIO;
> > > + }
> > > +
> > > + dev_dbg(dev, "PR done successfully\n");
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static enum fpga_mgr_states fme_mgr_state(struct fpga_manager *mgr)
> > > +{
> > > + return FPGA_MGR_STATE_UNKNOWN;
> > > +}
> > > +
> > > +static u64 fme_mgr_status(struct fpga_manager *mgr)
> > > +{
> > > + struct fme_mgr_priv *priv = mgr->priv;
> > > +
> > > + return pr_error_to_mgr_status(priv->pr_error);
> > > +}
> > > +
> > > +static const struct fpga_manager_ops fme_mgr_ops = {
> > > + .write_init = fme_mgr_write_init,
> > > + .write = fme_mgr_write,
> > > + .write_complete = fme_mgr_write_complete,
> > > + .state = fme_mgr_state,
> > > + .status = fme_mgr_status,
> > > +};
> > > +
> > > +static int fme_mgr_probe(struct platform_device *pdev)
> > > +{
> > > + struct device *dev = &pdev->dev;
> > > + struct fme_mgr_priv *priv;
> > > + struct fpga_manager *mgr;
> > > + struct resource *res;
> > > + int ret;
> > > +
> > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > + if (!priv)
> > > + return -ENOMEM;
> > > +
> > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > + priv->ioaddr = devm_ioremap(dev, res->start, resource_size(res));
> >
> > How about using devm_ioremap_resourc(dev, res) here instead?
>
> Actually the register region has already been mapped in lower level driver
> (e.g pci) so I think we don't have to map the second time here. I plan to
> add some code to pass the ioaddr via the platform data, and check if valid
> ioaddr from the platform data firstly in this probe function. If no pdata
> or no valid ioaddr, then go with devm_ioremap_resource. :)
If you end up sharing register spaces between drivers is regmap / syscon
maybe a good idea?
>
> >
> > > + if (IS_ERR(priv->ioaddr))
> > > + return PTR_ERR(priv->ioaddr);
> > > +
> > > + mgr = devm_kzalloc(dev, sizeof(*mgr), GFP_KERNEL);
> > > + if (!mgr)
> > > + return -ENOMEM;
> > > +
> > > + mgr->name = "DFL FPGA Manager";
> > > + mgr->mops = &fme_mgr_ops;
> > > + mgr->priv = priv;
> > > + mgr->parent = dev;
> > > + platform_set_drvdata(pdev, mgr);
> > > +
> > > + ret = fpga_mgr_register(mgr);
> > > + if (ret)
> > > + dev_err(dev, "unable to register FPGA manager\n");
> > > +
> > > + return ret;
> >
> > You can probably just do "return fpga_mgr_register(mgr);" here.
>
> Yes, it looks better, I will fix it. Thanks a lot for the review.
>
> Hao
>
> >
> > Thanks,
> > Alan
> >
> > > +}
> > > +
> > > +static int fme_mgr_remove(struct platform_device *pdev)
> > > +{
> > > + struct fpga_manager *mgr = platform_get_drvdata(pdev);
> > > +
> > > + fpga_mgr_unregister(mgr);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static struct platform_driver fme_mgr_driver = {
> > > + .driver = {
> > > + .name = FPGA_DFL_FME_MGR,
> > > + },
> > > + .probe = fme_mgr_probe,
> > > + .remove = fme_mgr_remove,
> > > +};
> > > +
> > > +module_platform_driver(fme_mgr_driver);
> > > +
> > > +MODULE_DESCRIPTION("FPGA Manager for DFL FPGA Management Engine");
> > > +MODULE_AUTHOR("Intel Corporation");
> > > +MODULE_LICENSE("GPL v2");
> > > +MODULE_ALIAS("platform:dfl-fme-mgr");
> > > --
> > > 2.7.4
> > >
Cheers,
Moritz
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox