diff mbox

[3/8] mka: pass full structures down to macsec drivers' transmit SC ops

Message ID 4783b95d242830fe43e1fafdcc31a162697f5b6e.1474298427.git.sd@queasysnail.net
State Changes Requested
Headers show

Commit Message

Sabrina Dubroca Sept. 20, 2016, 7:43 a.m. UTC 
Clean up the driver interface by passing pointers to struct transmit_sc
down the stack to the {create,delete}_transmit_sc ops, instead of passing the
individual arguments.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 src/drivers/driver.h            | 13 ++++++-------
 src/drivers/driver_macsec_qca.c | 13 +++++++------
 src/pae/ieee802_1x_kay.h        |  5 ++---
 src/pae/ieee802_1x_secy_ops.c   |  5 ++---
 wpa_supplicant/driver_i.h       | 12 +++++-------
 wpa_supplicant/wpas_kay.c       | 10 ++++------
 6 files changed, 26 insertions(+), 32 deletions(-)

Comments

Jouni Malinen Oct. 3, 2016, 10:11 a.m. UTC | #1 
On Tue, Sep 20, 2016 at 09:43:06AM +0200, Sabrina Dubroca wrote:
> Clean up the driver interface by passing pointers to struct transmit_sc
> down the stack to the {create,delete}_transmit_sc ops, instead of passing the
> individual arguments.

> diff --git a/src/drivers/driver.h b/src/drivers/driver.h

> -	int (*create_transmit_sc)(void *priv, u32 channel, const u8 *sci_addr,
> -				  u16 sci_port, unsigned int conf_offset);
> +	int (*create_transmit_sc)(void *priv, struct transmit_sc *sc,
> +				  enum confidentiality_offset conf_offset);

This changes conf_offset to an enum that has values 0..3.

> diff --git a/src/drivers/driver_macsec_qca.c b/src/drivers/driver_macsec_qca.c
> -static int macsec_qca_create_transmit_sc(void *priv, u32 channel,
> -					 const u8 *sci_addr, u16 sci_port,
> +static int macsec_qca_create_transmit_sc(void *priv, struct transmit_sc *sc,
>  					 unsigned int conf_offset)

However, the driver wrapper implementation here is not updated to match
that change in type. Nor was there any change on how the values are
handled.

> diff --git a/wpa_supplicant/wpas_kay.c b/wpa_supplicant/wpas_kay.c

>  static int
> -wpas_create_transmit_sc(void *wpa_s, u32 channel,
> -			const struct ieee802_1x_mka_sci *sci,
> +wpas_create_transmit_sc(void *wpa_s, struct transmit_sc *sc,
>  			enum confidentiality_offset co)
>  {
> -	return wpa_drv_create_transmit_sc(wpa_s, channel, sci->addr,
> -					  be_to_host16(sci->port),
> +	return wpa_drv_create_transmit_sc(wpa_s, sc,
>  					  conf_offset_val(co));
>  }

And this function is still converting that enum (values 0..3) to values
0, 30, or 50.

This cannot be correct.. Was the driver ops API really supposed to
change to the enum? If so, this conf_offset_val() in
wpas_create_transmit_sc() needs to be moved into
macsec_qca_create_transmit_sc().
Sabrina Dubroca Oct. 5, 2016, 8:43 a.m. UTC | #2 
2016-10-03, 13:11:26 +0300, Jouni Malinen wrote:
> On Tue, Sep 20, 2016 at 09:43:06AM +0200, Sabrina Dubroca wrote:
> > Clean up the driver interface by passing pointers to struct transmit_sc
> > down the stack to the {create,delete}_transmit_sc ops, instead of passing the
> > individual arguments.
> 
> > diff --git a/src/drivers/driver.h b/src/drivers/driver.h
> 
> > -	int (*create_transmit_sc)(void *priv, u32 channel, const u8 *sci_addr,
> > -				  u16 sci_port, unsigned int conf_offset);
> > +	int (*create_transmit_sc)(void *priv, struct transmit_sc *sc,
> > +				  enum confidentiality_offset conf_offset);
> 
> This changes conf_offset to an enum that has values 0..3.
> 
> > diff --git a/src/drivers/driver_macsec_qca.c b/src/drivers/driver_macsec_qca.c
> > -static int macsec_qca_create_transmit_sc(void *priv, u32 channel,
> > -					 const u8 *sci_addr, u16 sci_port,
> > +static int macsec_qca_create_transmit_sc(void *priv, struct transmit_sc *sc,
> >  					 unsigned int conf_offset)
> 
> However, the driver wrapper implementation here is not updated to match
> that change in type. Nor was there any change on how the values are
> handled.
> 
> > diff --git a/wpa_supplicant/wpas_kay.c b/wpa_supplicant/wpas_kay.c
> 
> >  static int
> > -wpas_create_transmit_sc(void *wpa_s, u32 channel,
> > -			const struct ieee802_1x_mka_sci *sci,
> > +wpas_create_transmit_sc(void *wpa_s, struct transmit_sc *sc,
> >  			enum confidentiality_offset co)
> >  {
> > -	return wpa_drv_create_transmit_sc(wpa_s, channel, sci->addr,
> > -					  be_to_host16(sci->port),
> > +	return wpa_drv_create_transmit_sc(wpa_s, sc,
> >  					  conf_offset_val(co));
> >  }
> 
> And this function is still converting that enum (values 0..3) to values
> 0, 30, or 50.
> 
> This cannot be correct.. Was the driver ops API really supposed to
> change to the enum? If so, this conf_offset_val() in
> wpas_create_transmit_sc() needs to be moved into
> macsec_qca_create_transmit_sc().

Oops, you're right, not sure how I managed to mess that up.  I'll
resubmit these patches without the change of type.


Thanks for the review,
diff mbox

Patch

diff --git a/src/drivers/driver.h b/src/drivers/driver.h
index 953c376f39b0..eca18105b018 100644
--- a/src/drivers/driver.h
+++ b/src/drivers/driver.h
@@ -3429,21 +3429,20 @@  struct wpa_driver_ops {
 	/**
 	 * create_transmit_sc - create secure connection for transmit
 	 * @priv: private driver interface data from init()
-	 * @channel: secure channel
-	 * @sci_addr: secure channel identifier - address
-	 * @sci_port: secure channel identifier - port
+	 * @sc: secure channel
+	 * @conf_offset: confidentiality offset
 	 * Returns: 0 on success, -1 on failure
 	 */
-	int (*create_transmit_sc)(void *priv, u32 channel, const u8 *sci_addr,
-				  u16 sci_port, unsigned int conf_offset);
+	int (*create_transmit_sc)(void *priv, struct transmit_sc *sc,
+				  enum confidentiality_offset conf_offset);
 
 	/**
 	 * delete_transmit_sc - delete secure connection for transmit
 	 * @priv: private driver interface data from init()
-	 * @channel: secure channel
+	 * @sc: secure channel
 	 * Returns: 0 on success, -1 on failure
 	 */
-	int (*delete_transmit_sc)(void *priv, u32 channel);
+	int (*delete_transmit_sc)(void *priv, struct transmit_sc *sc);
 
 	/**
 	 * create_transmit_sa - create secure association for transmit
diff --git a/src/drivers/driver_macsec_qca.c b/src/drivers/driver_macsec_qca.c
index 5a844cfd4c7b..4fe0a9ef913e 100644
--- a/src/drivers/driver_macsec_qca.c
+++ b/src/drivers/driver_macsec_qca.c
@@ -743,14 +743,14 @@  static int macsec_qca_get_available_transmit_sc(void *priv, u32 *channel)
 }
 
 
-static int macsec_qca_create_transmit_sc(void *priv, u32 channel,
-					 const u8 *sci_addr, u16 sci_port,
+static int macsec_qca_create_transmit_sc(void *priv, struct transmit_sc *sc,
 					 unsigned int conf_offset)
 {
 	struct macsec_qca_data *drv = priv;
 	int ret = 0;
 	fal_tx_class_lut_t entry;
 	u8 psci[ETH_ALEN + 2];
+	u32 channel = sc->channel;
 
 	wpa_printf(MSG_DEBUG, "%s: channel=%d", __func__, channel);
 
@@ -761,9 +761,9 @@  static int macsec_qca_create_transmit_sc(void *priv, u32 channel,
 	entry.action = FAL_TX_CLASS_ACTION_FORWARD;
 	entry.channel = channel;
 
-	os_memcpy(psci, sci_addr, ETH_ALEN);
-	psci[6] = (sci_port >> 8) & 0xf;
-	psci[7] = sci_port & 0xf;
+	os_memcpy(psci, sc->sci.addr, ETH_ALEN);
+	psci[6] = (sc->sci.port >> 8) & 0xf;
+	psci[7] = sc->sci.port & 0xf;
 
 	ret += nss_macsec_secy_tx_class_lut_set(drv->secy_id, channel, &entry);
 	ret += nss_macsec_secy_tx_sc_create(drv->secy_id, channel, psci, 8);
@@ -777,11 +777,12 @@  static int macsec_qca_create_transmit_sc(void *priv, u32 channel,
 }
 
 
-static int macsec_qca_delete_transmit_sc(void *priv, u32 channel)
+static int macsec_qca_delete_transmit_sc(void *priv, struct transmit_sc *sc)
 {
 	struct macsec_qca_data *drv = priv;
 	int ret = 0;
 	fal_tx_class_lut_t entry;
+	u32 channel = sc->channel;
 
 	wpa_printf(MSG_DEBUG, "%s: channel=%d", __func__, channel);
 
diff --git a/src/pae/ieee802_1x_kay.h b/src/pae/ieee802_1x_kay.h
index a747b11c7186..736c478bd6e0 100644
--- a/src/pae/ieee802_1x_kay.h
+++ b/src/pae/ieee802_1x_kay.h
@@ -156,14 +156,13 @@  struct ieee802_1x_kay_ctx {
 	int (*enable_receive_sa)(void *ctx, u32 channel, u8 an);
 	int (*disable_receive_sa)(void *ctx, u32 channel, u8 an);
 	int (*get_available_transmit_sc)(void *ctx, u32 *channel);
-	int (*create_transmit_sc)(void *ctx, u32 channel,
-				  const struct ieee802_1x_mka_sci *sci,
+	int (*create_transmit_sc)(void *ctx, struct transmit_sc *sc,
 				  enum confidentiality_offset co);
-	int (*delete_transmit_sc)(void *ctx, u32 channel);
 	int (*create_transmit_sa)(void *ctx, u32 channel, u8 an, u32 next_pn,
 				  Boolean confidentiality, const u8 *sak);
 	int (*enable_transmit_sa)(void *ctx, u32 channel, u8 an);
 	int (*disable_transmit_sa)(void *ctx, u32 channel, u8 an);
+	int (*delete_transmit_sc)(void *ctx, struct transmit_sc *sc);
 };
 
 struct ieee802_1x_kay {
diff --git a/src/pae/ieee802_1x_secy_ops.c b/src/pae/ieee802_1x_secy_ops.c
index d05e00f6f2cf..782e97927ab4 100644
--- a/src/pae/ieee802_1x_secy_ops.c
+++ b/src/pae/ieee802_1x_secy_ops.c
@@ -339,8 +339,7 @@  int secy_create_transmit_sc(struct ieee802_1x_kay *kay,
 		return -1;
 	}
 
-	return ops->create_transmit_sc(ops->ctx, txsc->channel, &txsc->sci,
-				       kay->co);
+	return ops->create_transmit_sc(ops->ctx, txsc, kay->co);
 }
 
 
@@ -361,7 +360,7 @@  int secy_delete_transmit_sc(struct ieee802_1x_kay *kay,
 		return -1;
 	}
 
-	return ops->delete_transmit_sc(ops->ctx, txsc->channel);
+	return ops->delete_transmit_sc(ops->ctx, txsc);
 }
 
 
diff --git a/wpa_supplicant/driver_i.h b/wpa_supplicant/driver_i.h
index 639bb83513c2..6442bfb8ecdf 100644
--- a/wpa_supplicant/driver_i.h
+++ b/wpa_supplicant/driver_i.h
@@ -837,23 +837,21 @@  wpa_drv_get_available_transmit_sc(struct wpa_supplicant *wpa_s, u32 *channel)
 }
 
 static inline int
-wpa_drv_create_transmit_sc(struct wpa_supplicant *wpa_s, u32 channel,
-			   const u8 *sci_addr, u16 sci_port,
-			   unsigned int conf_offset)
+wpa_drv_create_transmit_sc(struct wpa_supplicant *wpa_s, struct transmit_sc *sc,
+			   enum confidentiality_offset conf_offset)
 {
 	if (!wpa_s->driver->create_transmit_sc)
 		return -1;
-	return wpa_s->driver->create_transmit_sc(wpa_s->drv_priv, channel,
-						 sci_addr, sci_port,
+	return wpa_s->driver->create_transmit_sc(wpa_s->drv_priv, sc,
 						 conf_offset);
 }
 
 static inline int wpa_drv_delete_transmit_sc(struct wpa_supplicant *wpa_s,
-					     u32 channel)
+					     struct transmit_sc *sc)
 {
 	if (!wpa_s->driver->delete_transmit_sc)
 		return -1;
-	return wpa_s->driver->delete_transmit_sc(wpa_s->drv_priv, channel);
+	return wpa_s->driver->delete_transmit_sc(wpa_s->drv_priv, sc);
 }
 
 static inline int wpa_drv_create_transmit_sa(struct wpa_supplicant *wpa_s,
diff --git a/wpa_supplicant/wpas_kay.c b/wpa_supplicant/wpas_kay.c
index 306d9f189797..5650aa282249 100644
--- a/wpa_supplicant/wpas_kay.c
+++ b/wpa_supplicant/wpas_kay.c
@@ -143,19 +143,17 @@  static int wpas_get_available_transmit_sc(void *wpa_s, u32 *channel)
 
 
 static int
-wpas_create_transmit_sc(void *wpa_s, u32 channel,
-			const struct ieee802_1x_mka_sci *sci,
+wpas_create_transmit_sc(void *wpa_s, struct transmit_sc *sc,
 			enum confidentiality_offset co)
 {
-	return wpa_drv_create_transmit_sc(wpa_s, channel, sci->addr,
-					  be_to_host16(sci->port),
+	return wpa_drv_create_transmit_sc(wpa_s, sc,
 					  conf_offset_val(co));
 }
 
 
-static int wpas_delete_transmit_sc(void *wpa_s, u32 channel)
+static int wpas_delete_transmit_sc(void *wpa_s, struct transmit_sc *sc)
 {
-	return wpa_drv_delete_transmit_sc(wpa_s, channel);
+	return wpa_drv_delete_transmit_sc(wpa_s, sc);
 }