[PATCHv1] fpga: mgr: add FPGA configuration log

[PATCHv1] fpga: mgr: add FPGA configuration log

[richard.gong]

From: Richard Gong <richard.gong@intel.com>

Add a log for user to know FPGA configuration is successful

Signed-off-by: Richard Gong <richard.gong@intel.com>
---
 drivers/fpga/fpga-mgr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index c386681..559e046 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -151,6 +151,7 @@ static int fpga_mgr_write_complete(struct fpga_manager *mgr,
 	}
 	mgr->state = FPGA_MGR_STATE_OPERATING;
 
+	dev_info(&mgr->dev, "Successfully programming FPGA\n");
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCHv1] fpga: mgr: add FPGA configuration log

[Moritz Fischer]

Hi Richard,

On Tue, Apr 02, 2019 at 05:25:43PM -0500, richard.gong@linux.intel.com wrote:
> From: Richard Gong <richard.gong@intel.com>
> 
> Add a log for user to know FPGA configuration is successful
> 
> Signed-off-by: Richard Gong <richard.gong@intel.com>
> ---
>  drivers/fpga/fpga-mgr.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index c386681..559e046 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -151,6 +151,7 @@ static int fpga_mgr_write_complete(struct fpga_manager *mgr,
>  	}
>  	mgr->state = FPGA_MGR_STATE_OPERATING;
>  
> +	dev_info(&mgr->dev, "Successfully programming FPGA\n");

That info is available in FPGA manager's sysfs status entry, if at all
I'd make this a dev_dbg().

From my end I don't see how we need this really.

Thanks,
Moritz

Re: [PATCHv1] fpga: mgr: add FPGA configuration log

[Alan Tull]

On Wed, Apr 3, 2019 at 9:20 AM Moritz Fischer <mdf@kernel.org> wrote:
>
> Hi Richard,
>
> On Tue, Apr 02, 2019 at 05:25:43PM -0500, richard.gong@linux.intel.com wrote:
> > From: Richard Gong <richard.gong@intel.com>
> >
> > Add a log for user to know FPGA configuration is successful
> >
> > Signed-off-by: Richard Gong <richard.gong@intel.com>
> > ---
> >  drivers/fpga/fpga-mgr.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> > index c386681..559e046 100644
> > --- a/drivers/fpga/fpga-mgr.c
> > +++ b/drivers/fpga/fpga-mgr.c
> > @@ -151,6 +151,7 @@ static int fpga_mgr_write_complete(struct fpga_manager *mgr,
> >       }
> >       mgr->state = FPGA_MGR_STATE_OPERATING;
> >
> > +     dev_info(&mgr->dev, "Successfully programming FPGA\n");
>
> That info is available in FPGA manager's sysfs status entry, if at all
> I'd make this a dev_dbg().
>
> From my end I don't see how we need this really.

I'm ok with adding a message.  It's not adding lots of messages, just
one line for an event that will only happen for people who care about
the event (not too many FPGA users but if someone is using FPGA, they
will care about this.)

Alan


>
> Thanks,
> Moritz

Re: [PATCHv1] fpga: mgr: add FPGA configuration log

[Richard Gong]

Hi Moritz,


On 4/3/19 9:20 AM, Moritz Fischer wrote:
> Hi Richard,
> 
> On Tue, Apr 02, 2019 at 05:25:43PM -0500, richard.gong@linux.intel.com wrote:
>> From: Richard Gong <richard.gong@intel.com>
>>
>> Add a log for user to know FPGA configuration is successful
>>
>> Signed-off-by: Richard Gong <richard.gong@intel.com>
>> ---
>>   drivers/fpga/fpga-mgr.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
>> index c386681..559e046 100644
>> --- a/drivers/fpga/fpga-mgr.c
>> +++ b/drivers/fpga/fpga-mgr.c
>> @@ -151,6 +151,7 @@ static int fpga_mgr_write_complete(struct fpga_manager *mgr,
>>   	}
>>   	mgr->state = FPGA_MGR_STATE_OPERATING;
>>   
>> +	dev_info(&mgr->dev, "Successfully programming FPGA\n");
> 
> That info is available in FPGA manager's sysfs status entry, if at all
> I'd make this a dev_dbg().
> 
>  From my end I don't see how we need this really.

We got requests from the field and they want to see a log to get know if 
FPGA configuration is successfully completed. They don't want use any 
additional command to get status.

This log is useful for the user who performs FPGA configuration.

I think we need use dev_info, since dev_dbg is not enabled by fault for 
most build.

> 
> Thanks,
> Moritz
> 

Re: [PATCHv1] fpga: mgr: add FPGA configuration log

[Moritz Fischer]

Hi Richard,

On Wed, Apr 03, 2019 at 11:43:26AM -0500, Richard Gong wrote:
> Hi Moritz,
> 
> 
> On 4/3/19 9:20 AM, Moritz Fischer wrote:
> > Hi Richard,
> > 
> > On Tue, Apr 02, 2019 at 05:25:43PM -0500, richard.gong@linux.intel.com wrote:
> > > From: Richard Gong <richard.gong@intel.com>
> > > 
> > > Add a log for user to know FPGA configuration is successful
> > > 
> > > Signed-off-by: Richard Gong <richard.gong@intel.com>
> > > ---
> > >   drivers/fpga/fpga-mgr.c | 1 +
> > >   1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> > > index c386681..559e046 100644
> > > --- a/drivers/fpga/fpga-mgr.c
> > > +++ b/drivers/fpga/fpga-mgr.c
> > > @@ -151,6 +151,7 @@ static int fpga_mgr_write_complete(struct fpga_manager *mgr,
> > >   	}
> > >   	mgr->state = FPGA_MGR_STATE_OPERATING;
> > > +	dev_info(&mgr->dev, "Successfully programming FPGA\n");
> > 
> > That info is available in FPGA manager's sysfs status entry, if at all
> > I'd make this a dev_dbg().
> > 
> >  From my end I don't see how we need this really.
> 
> We got requests from the field and they want to see a log to get know if
> FPGA configuration is successfully completed. They don't want use any
> additional command to get status.
> 
> This log is useful for the user who performs FPGA configuration.
> 
> I think we need use dev_info, since dev_dbg is not enabled by fault for most
> build.

Well basically it boils down to:

$ dmesg | grep "Sucessfully"

vs

$ cat /sys/class/fpga.../status 

Personally not in favor of extra messages, but if we do it we should
change the message to "Sucessfully programmed FPGA".

I think making it a dbg message is a good trade-off ...

Moritz

Re: [PATCHv1] fpga: mgr: add FPGA configuration log

[Alan Tull]

On Wed, Apr 3, 2019 at 11:47 AM Moritz Fischer <mdf@kernel.org> wrote:
>
> Hi Richard,
>
> On Wed, Apr 03, 2019 at 11:43:26AM -0500, Richard Gong wrote:
> > Hi Moritz,
> >
> >
> > On 4/3/19 9:20 AM, Moritz Fischer wrote:
> > > Hi Richard,
> > >
> > > On Tue, Apr 02, 2019 at 05:25:43PM -0500, richard.gong@linux.intel.com wrote:
> > > > From: Richard Gong <richard.gong@intel.com>
> > > >
> > > > Add a log for user to know FPGA configuration is successful
> > > >
> > > > Signed-off-by: Richard Gong <richard.gong@intel.com>
> > > > ---
> > > >   drivers/fpga/fpga-mgr.c | 1 +
> > > >   1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> > > > index c386681..559e046 100644
> > > > --- a/drivers/fpga/fpga-mgr.c
> > > > +++ b/drivers/fpga/fpga-mgr.c
> > > > @@ -151,6 +151,7 @@ static int fpga_mgr_write_complete(struct fpga_manager *mgr,
> > > >           }
> > > >           mgr->state = FPGA_MGR_STATE_OPERATING;
> > > > + dev_info(&mgr->dev, "Successfully programming FPGA\n");
> > >
> > > That info is available in FPGA manager's sysfs status entry, if at all
> > > I'd make this a dev_dbg().
> > >
> > >  From my end I don't see how we need this really.
> >
> > We got requests from the field and they want to see a log to get know if
> > FPGA configuration is successfully completed. They don't want use any
> > additional command to get status.
> >
> > This log is useful for the user who performs FPGA configuration.
> >
> > I think we need use dev_info, since dev_dbg is not enabled by fault for most
> > build.
>
> Well basically it boils down to:
>
> $ dmesg | grep "Sucessfully"
>
> vs
>
> $ cat /sys/class/fpga.../status

it's state, not status for most fpga manager drivers.  It should
return 'operating' if everything went well.

It seems like there's a possible scenario where the FPGA starts up
with the FPGA in 'operating' mode and the user messes up early enough
that the state doesn't change.

>
> Personally not in favor of extra messages, but if we do it we should
> change the message to "Sucessfully programmed FPGA".
>
> I think making it a dbg message is a good trade-off ...
>
> Moritz

Re: [PATCHv1] fpga: mgr: add FPGA configuration log

[Alan Tull]

On Wed, Apr 3, 2019 at 1:05 PM Alan Tull <atull@kernel.org> wrote:
>
> On Wed, Apr 3, 2019 at 11:47 AM Moritz Fischer <mdf@kernel.org> wrote:
> >
> > Hi Richard,
> >
> > On Wed, Apr 03, 2019 at 11:43:26AM -0500, Richard Gong wrote:
> > > Hi Moritz,
> > >
> > >
> > > On 4/3/19 9:20 AM, Moritz Fischer wrote:
> > > > Hi Richard,
> > > >
> > > > On Tue, Apr 02, 2019 at 05:25:43PM -0500, richard.gong@linux.intel.com wrote:
> > > > > From: Richard Gong <richard.gong@intel.com>
> > > > >
> > > > > Add a log for user to know FPGA configuration is successful
> > > > >
> > > > > Signed-off-by: Richard Gong <richard.gong@intel.com>
> > > > > ---
> > > > >   drivers/fpga/fpga-mgr.c | 1 +
> > > > >   1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> > > > > index c386681..559e046 100644
> > > > > --- a/drivers/fpga/fpga-mgr.c
> > > > > +++ b/drivers/fpga/fpga-mgr.c
> > > > > @@ -151,6 +151,7 @@ static int fpga_mgr_write_complete(struct fpga_manager *mgr,
> > > > >           }
> > > > >           mgr->state = FPGA_MGR_STATE_OPERATING;
> > > > > + dev_info(&mgr->dev, "Successfully programming FPGA\n");
> > > >
> > > > That info is available in FPGA manager's sysfs status entry, if at all
> > > > I'd make this a dev_dbg().
> > > >
> > > >  From my end I don't see how we need this really.
> > >
> > > We got requests from the field and they want to see a log to get know if
> > > FPGA configuration is successfully completed. They don't want use any
> > > additional command to get status.
> > >
> > > This log is useful for the user who performs FPGA configuration.
> > >
> > > I think we need use dev_info, since dev_dbg is not enabled by fault for most
> > > build.
> >
> > Well basically it boils down to:
> >
> > $ dmesg | grep "Sucessfully"
> >
> > vs
> >
> > $ cat /sys/class/fpga.../status
>
> it's state, not status for most fpga manager drivers.  It should
> return 'operating' if everything went well.
>
> It seems like there's a possible scenario where the FPGA starts up
> with the FPGA in 'operating' mode and the user messes up early enough
> that the state doesn't change.
>
> >
> > Personally not in favor of extra messages, but if we do it we should
> > change the message to "Sucessfully programmed FPGA".
> >
> > I think making it a dbg message is a good trade-off ...

dbg vs info... On the one hand, it is a usually a message the
developer wants to see so the developer would turn on debug messages.
But then again FPGA programming doesn't happen that often and it is a
kind of significant event since it is your hardware changing i.e. it
won't add a lot messages, but it is sort of an important one if it
happens.   If the system crashes after a FPGA reprogramming event, it
would be good to have this in the log by default.  I don't want to
argue too powerfully for adding extra messages though.  Is this a case
where info is worth it since fpga programming is significant?

> >
> > Moritz

Re: [PATCHv1] fpga: mgr: add FPGA configuration log

[Moritz Fischer]

On Wed, Apr 03, 2019 at 01:37:51PM -0500, Alan Tull wrote:
> >
> > it's state, not status for most fpga manager drivers.  It should
> > return 'operating' if everything went well.

Yeah, sorry :)

> > It seems like there's a possible scenario where the FPGA starts up
> > with the FPGA in 'operating' mode and the user messes up early enough
> > that the state doesn't change.

Huh, then we should fix that instead? :)
> >
> > >
> > > Personally not in favor of extra messages, but if we do it we should
> > > change the message to "Sucessfully programmed FPGA".
> > >
> > > I think making it a dbg message is a good trade-off ...
> 
> dbg vs info... On the one hand, it is a usually a message the
> developer wants to see so the developer would turn on debug messages.
> But then again FPGA programming doesn't happen that often and it is a
> kind of significant event since it is your hardware changing i.e. it
> won't add a lot messages, but it is sort of an important one if it
> happens.   If the system crashes after a FPGA reprogramming event, it
> would be good to have this in the log by default.  I don't want to
> argue too powerfully for adding extra messages though.  Is this a case
> where info is worth it since fpga programming is significant?

In the current setup, it doesn't happen often. Going forward people
might have use-cases where this happens a lot more often.

I mean if y'all feel like this is required, sure, I still feel people
shouldn't rely on dmesg output for functional verification :)

I don't wanna guarantee that this message is gonna be there always ...

Cheers,
Moritz

Re: [PATCHv1] fpga: mgr: add FPGA configuration log

[Alan Tull]

On Wed, Apr 3, 2019 at 3:08 PM Moritz Fischer <mdf@kernel.org> wrote:
>
> On Wed, Apr 03, 2019 at 01:37:51PM -0500, Alan Tull wrote:
> > >
> > > it's state, not status for most fpga manager drivers.  It should
> > > return 'operating' if everything went well.
>
> Yeah, sorry :)
>
> > > It seems like there's a possible scenario where the FPGA starts up
> > > with the FPGA in 'operating' mode and the user messes up early enough
> > > that the state doesn't change.
>
> Huh, then we should fix that instead? :)
> > >
> > > >
> > > > Personally not in favor of extra messages, but if we do it we should
> > > > change the message to "Sucessfully programmed FPGA".
> > > >
> > > > I think making it a dbg message is a good trade-off ...
> >
> > dbg vs info... On the one hand, it is a usually a message the
> > developer wants to see so the developer would turn on debug messages.
> > But then again FPGA programming doesn't happen that often and it is a
> > kind of significant event since it is your hardware changing i.e. it
> > won't add a lot messages, but it is sort of an important one if it
> > happens.   If the system crashes after a FPGA reprogramming event, it
> > would be good to have this in the log by default.  I don't want to
> > argue too powerfully for adding extra messages though.  Is this a case
> > where info is worth it since fpga programming is significant?
>
> In the current setup, it doesn't happen often. Going forward people
> might have use-cases where this happens a lot more often.

Yes, then adding the message could become very spammy.

>
> I mean if y'all feel like this is required, sure, I still feel people
> shouldn't rely on dmesg output for functional verification :)

I agree with you that using sysfs to see what state the FPGA mgr ended
up in should be adequate most of the time.  We probably don't need
this.

>
> I don't wanna guarantee that this message is gonna be there always ...

Indeed...

Thanks,
Alan

>
> Cheers,
> Moritz