On Fri, Feb 15, 2019 at 4:19 AM Hannes Reinecke <hare@xxxxxxx> wrote:
>
> From: Sedat Dilek <sedat.dilek@xxxxxxxxx>
>
> commit 1917d42d14b7 ("fcoe: use enum for fip_mode") introduces a separate
> enum for the fip_mode that shall be used during initialisation handling
> until it is passed to fcoe_ctrl_link_up to set the initial fip_state.
> That change was incomplete and gcc quietly converted in various places
> between the fip_mode and the fip_state enum values with implicit enum
> conversions, which fortunately cannot cause any issues in the actual
> code's execution.
>
> clang however warns about these implicit enum conversions in the scsi
> drivers. This commit consolidates the use of the two enums, guided by
> clang's enum-conversion warnings.
>
> This commit now completes the use of the fip_mode:
> It expects and uses fip_mode in {bnx2fc,fcoe}_interface_create and
> fcoe_ctlr_init, and it calls fcoe_ctrl_set_set() with the correct
> values in fcoe_ctlr_link_up().
> It also breaks the association between FIP_MODE_AUTO and FIP_ST_AUTO
> to indicate these two enums are distinct.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/151
> Fixes: 1917d42d14b7 ("fcoe: use enum for fip_mode")
> Reported-by: Dmitry Golovin "Twisted Pair in my Hair" <dima@xxxxxxxxxx>
> CC: Lukas Bulwahn <lukas.bulwahn@xxxxxxxxx>
> CC: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
> CC: Nathan Chancellor <natechancellor@xxxxxxxxx>
> Suggested-by: Johannes Thumshirn <jthumshirn@xxxxxxx>
> Signed-off-by: Sedat Dilek <sedat.dilek@xxxxxxxxx>
> Signed-off-by: Hannes Reinecke <hare@xxxxxxxx>
> ---
> drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 2 +-
> drivers/scsi/fcoe/fcoe.c | 2 +-
> drivers/scsi/fcoe/fcoe_ctlr.c | 7 +++++--
> drivers/scsi/fcoe/fcoe_transport.c | 2 +-
> drivers/scsi/qedf/qedf_main.c | 2 +-
> include/scsi/libfcoe.h | 4 ++--
> 6 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
> index 2e4e7159ebf9..a75e74ad1698 100644
> --- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
> +++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
> @@ -1438,7 +1438,7 @@ static struct bnx2fc_hba *bnx2fc_hba_create(struct cnic_dev *cnic)
> static struct bnx2fc_interface *
> bnx2fc_interface_create(struct bnx2fc_hba *hba,
> struct net_device *netdev,
> - enum fip_state fip_mode)
> + enum fip_mode fip_mode)
> {
> struct fcoe_ctlr_device *ctlr_dev;
> struct bnx2fc_interface *interface;
> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> index cd19be3f3405..8ba8862d3292 100644
> --- a/drivers/scsi/fcoe/fcoe.c
> +++ b/drivers/scsi/fcoe/fcoe.c
> @@ -389,7 +389,7 @@ static int fcoe_interface_setup(struct fcoe_interface *fcoe,
> * Returns: pointer to a struct fcoe_interface or NULL on error
> */
> static struct fcoe_interface *fcoe_interface_create(struct net_device *netdev,
> - enum fip_state fip_mode)
> + enum fip_mode fip_mode)
> {
> struct fcoe_ctlr_device *ctlr_dev;
> struct fcoe_ctlr *ctlr;
> diff --git a/drivers/scsi/fcoe/fcoe_ctlr.c b/drivers/scsi/fcoe/fcoe_ctlr.c
> index 54da3166da8d..7dc4ffa24430 100644
> --- a/drivers/scsi/fcoe/fcoe_ctlr.c
> +++ b/drivers/scsi/fcoe/fcoe_ctlr.c
> @@ -147,7 +147,7 @@ static void fcoe_ctlr_map_dest(struct fcoe_ctlr *fip)
> * fcoe_ctlr_init() - Initialize the FCoE Controller instance
> * @fip: The FCoE controller to initialize
> */
> -void fcoe_ctlr_init(struct fcoe_ctlr *fip, enum fip_state mode)
> +void fcoe_ctlr_init(struct fcoe_ctlr *fip, enum fip_mode mode)
> {
> fcoe_ctlr_set_state(fip, FIP_ST_LINK_WAIT);
> fip->mode = mode;
> @@ -454,7 +454,10 @@ void fcoe_ctlr_link_up(struct fcoe_ctlr *fip)
> mutex_unlock(&fip->ctlr_mutex);
> fc_linkup(fip->lp);
> } else if (fip->state == FIP_ST_LINK_WAIT) {
> - fcoe_ctlr_set_state(fip, fip->mode);
> + if (fip->mode == FIP_MODE_NON_FIP)
> + fcoe_ctlr_set_state(fip, FIP_ST_NON_FIP);
> + else
> + fcoe_ctlr_set_state(fip, FIP_ST_AUTO);
> switch (fip->mode) {
> default:
> LIBFCOE_FIP_DBG(fip, "invalid mode %d\n", fip->mode);
> diff --git a/drivers/scsi/fcoe/fcoe_transport.c b/drivers/scsi/fcoe/fcoe_transport.c
> index f4909cd206d3..b381ab066b9c 100644
> --- a/drivers/scsi/fcoe/fcoe_transport.c
> +++ b/drivers/scsi/fcoe/fcoe_transport.c
> @@ -873,7 +873,7 @@ static int fcoe_transport_create(const char *buffer,
> int rc = -ENODEV;
> struct net_device *netdev = NULL;
> struct fcoe_transport *ft = NULL;
> - enum fip_state fip_mode = (enum fip_state)(long)kp->arg;
> + enum fip_mode fip_mode = (enum fip_mode)(long)kp->arg;
Thanks so much for picking this up Hannes! Sorry I didn't comment on
this until v4, but is the intermediate cast to long (before casting
again to the enum) necessary? Aren't vanilla enums `int`s, not
`long`s? Otherwise this patches looks mostly good to go.
>
> mutex_lock(&ft_mutex);
>
> diff --git a/drivers/scsi/qedf/qedf_main.c b/drivers/scsi/qedf/qedf_main.c
> index 9bbc19fc190b..9f9431a4cc0e 100644
> --- a/drivers/scsi/qedf/qedf_main.c
> +++ b/drivers/scsi/qedf/qedf_main.c
> @@ -1418,7 +1418,7 @@ static struct libfc_function_template qedf_lport_template = {
>
> static void qedf_fcoe_ctlr_setup(struct qedf_ctx *qedf)
> {
> - fcoe_ctlr_init(&qedf->ctlr, FIP_ST_AUTO);
> + fcoe_ctlr_init(&qedf->ctlr, FIP_MODE_AUTO);
>
> qedf->ctlr.send = qedf_fip_send;
> qedf->ctlr.get_src_addr = qedf_get_src_mac;
> diff --git a/include/scsi/libfcoe.h b/include/scsi/libfcoe.h
> index cb8a273732cf..bb8092fa1e36 100644
> --- a/include/scsi/libfcoe.h
> +++ b/include/scsi/libfcoe.h
> @@ -79,7 +79,7 @@ enum fip_state {
> * It must not change after fcoe_ctlr_init() sets it.
> */
> enum fip_mode {
> - FIP_MODE_AUTO = FIP_ST_AUTO,
> + FIP_MODE_AUTO,
> FIP_MODE_NON_FIP,
> FIP_MODE_FABRIC,
> FIP_MODE_VN2VN,
> @@ -250,7 +250,7 @@ struct fcoe_rport {
> };
>
> /* FIP API functions */
> -void fcoe_ctlr_init(struct fcoe_ctlr *, enum fip_state);
> +void fcoe_ctlr_init(struct fcoe_ctlr *, enum fip_mode);
> void fcoe_ctlr_destroy(struct fcoe_ctlr *);
> void fcoe_ctlr_link_up(struct fcoe_ctlr *);
> int fcoe_ctlr_link_down(struct fcoe_ctlr *);
> --
> 2.16.4
>
--
Thanks,
~Nick Desaulniers
