| Message ID | 20190215121920.1780-1-hare@suse.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [PATCHv4] fcoe: make use of fip_mode enum complete | expand |
On Fri, Feb 15, 2019 at 4:19 AM Hannes Reinecke <hare@suse.de> wrote: > > From: Sedat Dilek <sedat.dilek@gmail.com> > > 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@golovin.in> > CC: Lukas Bulwahn <lukas.bulwahn@gmail.com> > CC: Nick Desaulniers <ndesaulniers@google.com> > CC: Nathan Chancellor <natechancellor@gmail.com> > Suggested-by: Johannes Thumshirn <jthumshirn@suse.de> > Signed-off-by: Sedat Dilek <sedat.dilek@gmail.com> > Signed-off-by: Hannes Reinecke <hare@suse.com> > --- > 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 >
On Fri, Feb 15, 2019 at 01:19:20PM +0100, Hannes Reinecke wrote: > From: Sedat Dilek <sedat.dilek@gmail.com> > > 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@golovin.in> > CC: Lukas Bulwahn <lukas.bulwahn@gmail.com> > CC: Nick Desaulniers <ndesaulniers@google.com> > CC: Nathan Chancellor <natechancellor@gmail.com> > Suggested-by: Johannes Thumshirn <jthumshirn@suse.de> > Signed-off-by: Sedat Dilek <sedat.dilek@gmail.com> > Signed-off-by: Hannes Reinecke <hare@suse.com> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> Tested-by: Nathan Chancellor <natechancellor@gmail.com> As a side note, I think Lukas deserves a little more credit than just a CC, most of the commit message wording and patch content is his. At the least, a link to the original patch: https://lore.kernel.org/lkml/20190112053427.35696-1-lukas.bulwahn@gmail.com/ Cheers, Nathan > --- > 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; > > 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 >
On Fri, Feb 15, 2019 at 6:35 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > > On Fri, Feb 15, 2019 at 01:19:20PM +0100, Hannes Reinecke wrote: > > From: Sedat Dilek <sedat.dilek@gmail.com> > > > > 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@golovin.in> > > CC: Lukas Bulwahn <lukas.bulwahn@gmail.com> > > CC: Nick Desaulniers <ndesaulniers@google.com> > > CC: Nathan Chancellor <natechancellor@gmail.com> > > Suggested-by: Johannes Thumshirn <jthumshirn@suse.de> > > Signed-off-by: Sedat Dilek <sedat.dilek@gmail.com> > > Signed-off-by: Hannes Reinecke <hare@suse.com> > > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > Tested-by: Nathan Chancellor <natechancellor@gmail.com> > > As a side note, I think Lukas deserves a little more credit than just a > CC, most of the commit message wording and patch content is his. At the > least, a link to the original patch: > > https://lore.kernel.org/lkml/20190112053427.35696-1-lukas.bulwahn@gmail.com/ > Hi, I had a laptop-free weekend and answering now. Of course Lukas deserves his credits for his initial work. In [1] had all references - guess it got lost when the patch was applied. AFAICS the patch has changed and whatever Hannes thinks it's OK to do. Hope it gets pushed soon. For me, dealing with FCOE revealed a reproducible compiler-crash and was a good test-case if the llvm-toolchain is fine. Thanks to all involved people. Best Regards, - Sedat - [1] https://marc.info/?l=linux-scsi&m=154990531419036&w=2 > Cheers, > Nathan > > > --- > > 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; > > > > 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 > >
On Mon, Feb 18, 2019 at 8:58 AM Sedat Dilek <sedat.dilek@gmail.com> wrote: > > On Fri, Feb 15, 2019 at 6:35 PM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > On Fri, Feb 15, 2019 at 01:19:20PM +0100, Hannes Reinecke wrote: > > > From: Sedat Dilek <sedat.dilek@gmail.com> > > > > > > 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@golovin.in> > > > CC: Lukas Bulwahn <lukas.bulwahn@gmail.com> > > > CC: Nick Desaulniers <ndesaulniers@google.com> > > > CC: Nathan Chancellor <natechancellor@gmail.com> > > > Suggested-by: Johannes Thumshirn <jthumshirn@suse.de> > > > Signed-off-by: Sedat Dilek <sedat.dilek@gmail.com> > > > Signed-off-by: Hannes Reinecke <hare@suse.com> > > > > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > > Tested-by: Nathan Chancellor <natechancellor@gmail.com> > > > > As a side note, I think Lukas deserves a little more credit than just a > > CC, most of the commit message wording and patch content is his. At the > > least, a link to the original patch: > > > > https://lore.kernel.org/lkml/20190112053427.35696-1-lukas.bulwahn@gmail.com/ > > > > Hi, > > I had a laptop-free weekend and answering now. > > Of course Lukas deserves his credits for his initial work. > > In [1] had all references - guess it got lost when the patch was applied. > > AFAICS the patch has changed and whatever Hannes thinks it's OK to do. > Hope it gets pushed soon. > > For me, dealing with FCOE revealed a reproducible compiler-crash and > was a good test-case if the llvm-toolchain is fine. > > Thanks to all involved people. > > Best Regards, > - Sedat - > > > [1] https://marc.info/?l=linux-scsi&m=154990531419036&w=2 > > > Cheers, > > Nathan [ ... ] Now, I remember what went wrong. I have sent this patch proposal from my notebook at work with its Git setup (company's email-address). To receive it and all answers to my private email-adresss I had added the "From: ..." line. Of course, the text in the commit message should have been embedded with "Lukas Bulwahn writes: ...". I will look how to switch easily my Git setup here. So, my apologies. - Sedat -
On Mon, 18 Feb 2019, Sedat Dilek wrote: > On Fri, Feb 15, 2019 at 6:35 PM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > On Fri, Feb 15, 2019 at 01:19:20PM +0100, Hannes Reinecke wrote: > > > From: Sedat Dilek <sedat.dilek@gmail.com> > > > > > > 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@golovin.in> > > > CC: Lukas Bulwahn <lukas.bulwahn@gmail.com> > > > CC: Nick Desaulniers <ndesaulniers@google.com> > > > CC: Nathan Chancellor <natechancellor@gmail.com> > > > Suggested-by: Johannes Thumshirn <jthumshirn@suse.de> > > > Signed-off-by: Sedat Dilek <sedat.dilek@gmail.com> > > > Signed-off-by: Hannes Reinecke <hare@suse.com> > > > > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > > Tested-by: Nathan Chancellor <natechancellor@gmail.com> > > > > As a side note, I think Lukas deserves a little more credit than just a > > CC, most of the commit message wording and patch content is his. At the > > least, a link to the original patch: > > > > https://lore.kernel.org/lkml/20190112053427.35696-1-lukas.bulwahn@gmail.com/ > > > > Hi, > > I had a laptop-free weekend and answering now. > > Of course Lukas deserves his credits for his initial work. > > In [1] had all references - guess it got lost when the patch was applied. > > AFAICS the patch has changed and whatever Hannes thinks it's OK to do. > Hope it gets pushed soon. > Sorry for the late response and dropping the ball on this patch. Looking at clang warnings is rather an hobbyist activity for me besides my daily job. Feel free to assign authorship to me or not; I have no strong opinion on that. Getting clang established as equal compiler to gcc for the kernel is the goal we all share. > For me, dealing with FCOE revealed a reproducible compiler-crash and > was a good test-case if the llvm-toolchain is fine. > > Thanks to all involved people. > > Best Regards, > - Sedat - > > > [1] https://marc.info/?l=linux-scsi&m=154990531419036&w=2 > > > Cheers, > > Nathan > > > > > --- > > > 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; My original patch already had this duplicated conversion, as I wanted to keep my change minimal invasive and I really have little understanding of the FCoE functionality. But Nick is right, we can probably simplify this to one conversion. It seems to have been overly complicated in the initial implementation anyway. Lukas > > > > > > 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 > > > >
Hannes, > 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. Applied to 5.1/scsi-queue, thanks!
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; 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 *);