On Mon, Sep 14, 2020 at 3:24 PM Stafford Horne [off-list ref] wrote:
On Mon, Sep 14, 2020 at 12:33:11PM +0200, Mateusz Holenko wrote:
quoted
On Fri, Sep 11, 2020 at 2:57 AM Stafford Horne [off-list ref] wrote:
quoted
On Wed, Aug 12, 2020 at 02:34:34PM +0200, Mateusz Holenko wrote:
quoted
From: Pawel Czarnecki <redacted>
This commit adds driver for the FPGA-based LiteX SoC
Controller from LiteX SoC builder.
Co-developed-by: Mateusz Holenko <mholenko@antmicro.com>
Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
Signed-off-by: Pawel Czarnecki <redacted>
---
+ node = dev->of_node;
+ if (!node)
+ return -ENODEV;
We return here without BUG() if the setup fails.
quoted
quoted
quoted
+
+ soc_ctrl_dev = devm_kzalloc(dev, sizeof(*soc_ctrl_dev), GFP_KERNEL);
+ if (!soc_ctrl_dev)
+ return -ENOMEM;
We return here without BUG() if we are out of memory.
quoted
quoted
quoted
+
+ soc_ctrl_dev->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(soc_ctrl_dev->base))
+ return PTR_ERR(soc_ctrl_dev->base);
Etc.
You are totally right - this is not consistent.
We should probably either trigger BUG() in each case or don't bother at all.
quoted
quoted
quoted
+
+ result = litex_check_csr_access(soc_ctrl_dev->base);
+ if (result) {
+ // LiteX CSRs access is broken which means that
+ // none of LiteX drivers will most probably
+ // operate correctly
The comment format here with // is not usually used in the kernel, but its not
forbidded. Could you use the /* */ multiline style?
Sure, I'll change the commenting style here.
quoted
quoted
+ BUG();
Instead of stopping the system with BUG, could we just do:
return litex_check_csr_access(soc_ctrl_dev->base);
We already have failure for NODEV/NOMEM so might as well not call BUG() here
too.
It's true that litex_check_csr_accessors() already generates error
codes that could be
returned directly.
The point of using BUG() macro here, however, is to stop booting the
system so that it's visible
(and impossible to miss for the user) that an unresolvable HW issue
was encountered.
CSR-accessors - the litex_{g,s}et_reg() functions - are intended to be
used by other LiteX drivers
and it's very unlikely that those drivers would work properly after
the fail of litex_check_csr_accessors().
Since in such case the UART driver will be affected too (no boot logs
and error messages visible to the user),
I thought it'll be easier to spot and debug the problem if the system
stopped in the BUG loop.
Perhaps there are other, more linux-friendly, ways of achieving a
similar goal - I'm open for suggestions.
I see your point, but I thought if failed with an exit status above, we could do
the same here. But I guess failing here means that something is really wrong as
validation failed.
Some points:
- If we return here, the system will still boot but there will be no UART
- If we bail with BUG(), here the system stops, and there is no UART
- Both cases the user can connect with a debugger and read "dmesg", to see what
is wrong, but BUG() does not print an error message on all architectures.
We could also use:
- WARN(1, "Failed to validate CSR registers, the system is probably broken.");
If you want to keep BUG() it may be fine.
I am not an expert on handling these type of bailout's so other input is
appreciated.
I don't have a strong opinion about using BUG() here - I just thought
it would be easier for the user.
If this is, however, not how linux typically works, I'm ok with
reworking this part.
-Stafford
Best,
Mateusz
--
Mateusz Holenko
Antmicro Ltd | www.antmicro.com
Roosevelta 22, 60-829 Poznan, Poland